feat: setup `hooks` package #31

Dhaiwat10 · 2021-11-29T09:30:21Z

This PR initalizes the hooks package and adds some starter code in the form of a useWallet hook.

Known issues

I tried using the useWallet hook inside the storybook for the components package. It did not work. For some reason, when I log useWallet's contents all properties come out as undefined. It works fine in the storybook for the hooks package, though. Feel free to try both environments out yourself.

Other changes

Removed an unnecessary story from the components package
Updated the story for Address in thecomponents package
Updated some scripts in the root package.json

Notes

I published v0.1.0 of @web3-ui/hooks to test out if there's a problem with the bundling and stuff is just not being compiled.

cc @etr2460

naz3eh · 2021-11-29T13:21:46Z

I tried this out.
While running yarn, it gave me the below error.

Then I upgraded my node to version 16 and the error disappeared.
Except that everything LGTM.

Though I'd love to add more hooks.

Canopix · 2021-11-29T13:56:32Z

Great Work @Dhaiwat10 🚀

with-heart · 2021-11-29T15:19:09Z

package.json

@@ -2,7 +2,7 @@
  "name": "@web3-ui/root",
  "license": "MIT",
  "version": "0.0.0",
-  "private": "true",
+  "private": "false",


We want this to stay private, right? This should never be published because it's just a way to coordinate the other packages.

etr2460

a few comments, nothing blocking, but mostly asking about how we want to handle the monorepo setup

etr2460 · 2021-11-29T15:56:28Z

.gitignore

+/coverage
+
+dist


nit: let's add a newline to the end of the file

also, /dist/ is already used on line 3. let's remove that one and replace it with dist instead

etr2460 · 2021-11-29T15:59:21Z

package.json

+    "build:components": "yarn workspace @web3-ui/components build",
+    "build:hooks": "yarn workspace @web3-ui/hooks build",


I think i mentioned this in the tests pr i mad, but it might be cleaner to share the build scripts and configs between he two packages. so we'd have a build command in the root json that runs rollup, and then you could pass in either packages/components or packages/hooks to the command to build a specific package, or it would just build both if nothing is passed. thoughts?

@etr2460 that's def a good idea but I suggest creating a separate issue/PR for that. Thoughts?

Added this to the list in #33

etr2460 · 2021-11-29T16:00:08Z

package.json

+    "dev:components": "yarn workspace @web3-ui/components dev",
+    "dev:hooks": "yarn workspace @web3-ui/hooks dev"


same comment as above for the storybook setup here. if we can share configuration, then we'll reduce the code and make it easier to apply updates/config changes to both packages without missing one

Created #32 and added to the list in #33

etr2460 · 2021-11-29T16:01:54Z

packages/components/src/components/Address/Address.stories.tsx


-storiesOf('Address', module).add('Default', () => <Address />);
+storiesOf('Address', module).add('Default', () => <Address value='This is an input' />);


minor nit, but i like my storybook components to contain real looking content. So something like this might look better (and make it more clear what the component is for)

Suggested change

storiesOf('Address', module).add('Default', () => <Address value='This is an input' />);

storiesOf('Address', module).add('Default', () => <Address value='testaddress.eth' />);

etr2460 · 2021-11-29T16:02:26Z

packages/hooks/.prettierrc.js

@@ -0,0 +1,7 @@
+module.exports = {


should prettier files be consolidated in the root of the monorepo too?

Added to #33

etr2460 · 2021-11-29T16:04:22Z

packages/hooks/package.json

+    "test:watch": "yarn test --watch",
+    "test:coverage": "jest --coverage --colors --maxWorkers=2",
+    "hygen": "hygen",
+    "component:new": "hygen component with-prompt",


do we need this? it looks specific to components

Probably not. I'll remove it

etr2460 · 2021-11-29T16:06:22Z

packages/hooks/src/Provider.tsx

+  connectWallet: (() => void) | undefined;
+  signer: JsonRpcSigner | null | undefined;
+  userAddress: string | null | undefined;
+  disconnectWallet: (() => void) | undefined;
+  network: string;
+  chainId: number | undefined;
+  connected: boolean;
+  provider: ethers.providers.Web3Provider | null | undefined;


this is a styling preference, so no need to accept, but i find it easier to grok these when the shorthand for undefined is used, like this:

Suggested change

connectWallet: (() => void) | undefined;

signer: JsonRpcSigner | null | undefined;

userAddress: string | null | undefined;

disconnectWallet: (() => void) | undefined;

network: string;

chainId: number | undefined;

connected: boolean;

provider: ethers.providers.Web3Provider | null | undefined;

connectWallet?: (() => void);

signer?: JsonRpcSigner | null;

userAddress?: string | null;

disconnectWallet?: (() => void);

network: string;

chainId?: number;

connected: boolean;

provider?: ethers.providers.Web3Provider | null;

Yeah that def needs to be changed. Just laziness on my part. Wanted to ship this PR quick.

etr2460 · 2021-11-29T16:08:13Z

packages/hooks/src/Provider.tsx

+  const [signer, setSigner] = React.useState<null | JsonRpcSigner>(null);
+  const [provider, setProvider] = React.useState<null | ethers.providers.Web3Provider>(null);
+  const [userAddress, setUserAddress] = React.useState<null | string>(null);
+  const [web3Modal, setWeb3Modal] = React.useState<null | Web3Modal>();


what's the difference between null and undefined here? if this is only ever undefined or Web3Modal, then lets remove the null type

etr2460 · 2021-11-29T16:08:58Z

packages/hooks/src/hooks/useWallet.ts

+      const { userAddress, provider } = context;
+      if (userAddress && provider) {
+        provider.lookupAddress(userAddress).then((address) => {
+          setEns(address as string);


do we need to handle an error case where the address isn't found?

No. In that case, the ens will simply be null which is fine.

etr2460 · 2021-11-29T16:10:12Z

packages/hooks/tsconfig.json

@@ -0,0 +1,23 @@
+{


if #25 goes in, then this file will probably get deleted too in favor of the root version

with-heart

A few suggestions but I think we could also just move forward with this and revisit things later. Whatever you wanna do 😁

with-heart · 2021-11-29T15:22:29Z

packages/hooks/.prettierrc.js

+module.exports = {
+  printWidth: 100,
+  semi: true,
+  singleQuote: true,
+  trailingComma: 'es5',
+  jsxSingleQuote: true,
+};


We only need one root .prettierrc for this to work across the workspace.

Also I think we should try to stick with the same config we've been using in the other projects just to make things consistent! printWidth: 100 and jsxSingleQuote: true are pretty non-standard generally imo

Great suggestion. I think this can be coupled with some other devops-y changes @etr2460 suggested in his reviews. We can batch these change together in a PR dedicated to that.

with-heart · 2021-11-29T15:54:27Z

packages/hooks/.storybook/main.js

+module.exports = {
+  stories: ['../src/**/*.stories.mdx', '../src/**/*.stories.@(js|jsx|ts|tsx)'],
+  addons: ['@storybook/addon-links', '@storybook/addon-essentials'],
+  framework: '@storybook/react',
+};


I think we should combine hooks/.storybook and components/.storybook into a single Storybook instance at the root. We can leave this for now though and tackle combining them in #32

with-heart · 2021-11-29T15:56:08Z

packages/hooks/package.json

+    "test": "jest --maxWorkers=2",
+    "test:watch": "yarn test --watch",
+    "test:coverage": "jest --coverage --colors --maxWorkers=2",
+    "hygen": "hygen",


I think we should remove hygen from this PR and add it in a follow-up so we can discuss and document it. Seems interesting but it deserves its own space so we can all be on the same page about what it is and how to use it.

with-heart · 2021-11-29T16:02:56Z

packages/hooks/package.json

+    "test:coverage": "jest --coverage --colors --maxWorkers=2",
+    "hygen": "hygen",
+    "component:new": "hygen component with-prompt",
+    "pre-commit-hook": "yarn lint-staged",


We shouldn't need this here since we have the config at the root. I'm fine with moving forward with this and resolving it later as part of #33 though

with-heart · 2021-11-29T16:06:00Z

package.json

+    "dev:components": "yarn workspace @web3-ui/components dev",
+    "dev:hooks": "yarn workspace @web3-ui/hooks dev"


imo it'd be more familiar to call these storybook:components and storybook:hooks. dev is typically associated with running a development server which is sort of what Storybook is, but I think a lot of projects conceptualize storybook as a separate thing.

Running yarn storybook:components makes it pretty clear what you're running whereas dev is pretty ambiguous and it starting Storybook would imo be surprising to most

with-heart · 2021-11-29T16:19:31Z

packages/hooks/src/Provider.tsx

+  const value = React.useMemo(
+    () => ({
+      connectWallet,
+      signer,
+      userAddress,
+      disconnectWallet,
+      connected,
+      provider,
+      network,
+      chainId,
+    }),
+    [connectWallet, signer, userAddress, web3Modal, connected, provider, network, chainId]
+  );


useMemo probably isn't helping in the way you think it is.

Some resources that might explain why/provide alternatives:

https://github.com/kentcdodds/old-kentcdodds.com/blob/319db97260078ea4c263e75166f05e2cea21ccd1/content/blog/how-to-optimize-your-context-value/index.md

Preventing rerenders with React.memo and useContext hook. facebook/react#15156 (comment)

https://kentcdodds.com/blog/how-to-optimize-your-context-value

I think we should just go with this for now and figure out a better pattern later, just wanted to throw that out there :)

Thanks for sharing these resources. Super helpful

with-heart · 2021-11-29T16:23:40Z

packages/hooks/src/hooks/index.ts

@@ -0,0 +1 @@
+export * from './useWallet';


One thing I've learned from experience is that it's better to be explicit with re-exports in index files:

Suggested change

export * from './useWallet';

export { useWallet } from './useWallet';

Anyone looking at this file can tell at a glance what is exported from this folder, whereas export * syntax requires them to read through each individual file to know what all is exported.

with-heart · 2021-11-29T16:25:39Z

packages/hooks/src/Provider.tsx

+import WalletConnectProvider from '@walletconnect/web3-provider';
+import Web3Modal from 'web3modal';
+
+export interface Web3ContextType {


Web3ContextType

But it's not a type, it's an interface! 😆

with-heart · 2021-11-29T16:27:51Z

packages/hooks/src/Provider.tsx

+  provider: ethers.providers.Web3Provider | null | undefined;
+}
+
+export const Web3Context = React.createContext<Web3ContextType | undefined>(undefined);


We'll probably want to use a few tricks for defining this context so we (or our users) don't have to check for its existence all over the place before we can use it.

// without tricks const web3Context = useContext(Web3Context) // gross web3Context?.connectWallet() // with tricks const web3Context = useContext(Web3Context) // yum web3Context.connectWallet()

We'll get to it later!

Yeah this is a massive pain point.

with-heart · 2021-11-29T16:28:03Z

packages/hooks/src/index.ts

+export * from './hooks';
+export * from './Provider';


Similar to previous comment:

Suggested change

export * from './hooks';

export * from './Provider';

export { useWallet } from './hooks';

export {

Provider,

ProviderProps,

Web3Context,

Web3ContextType,

} from './Provider';

Dhaiwat10 requested review from with-heart and crondinini November 29, 2021 09:30

Dhaiwat10 force-pushed the feat/hooks-package branch from 8225a18 to ef1b91e Compare November 29, 2021 09:32

Dhaiwat10 linked an issue Nov 29, 2021 that may be closed by this pull request

Setup the hooks package with some basic hooks #30

Closed

Canopix approved these changes Nov 29, 2021

View reviewed changes

with-heart reviewed Nov 29, 2021

View reviewed changes

etr2460 reviewed Nov 29, 2021

View reviewed changes

with-heart approved these changes Nov 29, 2021

View reviewed changes

feat: setup hooks package

4aa5d72

Dhaiwat10 force-pushed the feat/hooks-package branch from ef1b91e to 4aa5d72 Compare November 29, 2021 18:39

Dhaiwat10 merged commit e08bfe4 into master Nov 29, 2021

Dhaiwat10 deleted the feat/hooks-package branch December 6, 2021 07:13

		"build:components": "yarn workspace @web3-ui/components build",
		"build:hooks": "yarn workspace @web3-ui/hooks build",

		"dev:components": "yarn workspace @web3-ui/components dev",
		"dev:hooks": "yarn workspace @web3-ui/hooks dev"


		storiesOf('Address', module).add('Default', () => <Address />);
		storiesOf('Address', module).add('Default', () => <Address value='This is an input' />);

	export * from './useWallet';
	export { useWallet } from './useWallet';

-export * from './hooks';
-export * from './Provider';
+export { useWallet } from './hooks';
+export {
+  Provider,
+  ProviderProps,
+  Web3Context,
+  Web3ContextType,
+} from './Provider';

		/coverage

		dist

feat: setup hooks package #31

feat: setup hooks package #31

Conversation

Dhaiwat10 commented Nov 29, 2021 • edited Loading

Known issues

Other changes

Notes

naz3eh commented Nov 29, 2021 • edited Loading

Canopix commented Nov 29, 2021

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

with-heart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

with-heart Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: setup `hooks` package #31

feat: setup `hooks` package #31

Dhaiwat10 commented Nov 29, 2021 •

edited

Loading

naz3eh commented Nov 29, 2021 •

edited

Loading

with-heart Nov 29, 2021 •

edited

Loading