Skip to content

Conversation

@iamacook
Copy link

@iamacook iamacook commented Apr 2, 2022

Description

This PR simplifies state subscription by using React 18 and the new useSyncExternalStore hook, as well as a few small tweaks.

  1. Instead of manually subscribing to the state in each hook, a new useAppState hook has been added. It's possible to subscribe to the entire state or part of it via its argument, just as one would with the select() method.
  2. The options argument of the connect() method has been made optional to match that of connectWallet().
  3. Unnecessary casting has been removed by destructuring methods from the web3Onboard instance directly.

I haven't altered the version number as I'm not sure how you guys would like to approach that seeing as it includes a substantial dependency upgrade.

Checklist

  • The version field in package.json is incremented following semantic versioning
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works
  • This PR passes the Circle CI checks

@lnbc1QWFyb24
Copy link
Contributor

Awesome, thanks for this @iamacook! We will review asap.

@lnbc1QWFyb24
Copy link
Contributor

Hey @iamacook would you consider this a breaking change? I am trying to get a sense of how big a deal it is to force apps to update to v18 of React if they are not already upgraded. It seems like it can be a breaking change in some rare cases with other React libraries from reading the React docs on v18? Ideally we would release this as a minor version update.

@iamacook
Copy link
Author

Hey @iamacook would you consider this a breaking change? I am trying to get a sense of how big a deal it is to force apps to update to v18 of React if they are not already upgraded. It seems like it can be a breaking change in some rare cases with other React libraries from reading the React docs on v18? Ideally we would release this as a minor version update.

As React 18 is a direct dependency of the package, it shouldn't be a breaking change. However, I'll double-check to be sure and get back to you ASAP.

@iamacook
Copy link
Author

@aaronbarnardsound, sorry for taking a while to get back to you.

I've added the official shim and adjusted the dependencies accordingly. The shim (use-sync-external-store) is only used if the user doesn't have the latest version of React.

@ChristopherWirtOfficial

@aaronbarnardsound Can this be prioritized? Currently, the package won't work with React 18 because of strict peerDependancies.

@lnbc1QWFyb24
Copy link
Contributor

@ChristopherWirtOfficial Yes this should be merged shortly and is currently in review!

@mahmud-bn mahmud-bn changed the base branch from v2-web3-onboard to v2-web3-onboard-develop May 19, 2022 20:12
@mahmud-bn
Copy link
Contributor

Hi @iamacook,
I'm currently working on merging your PR.

  • I changed the base branch and also merged your PR with v2-web3-onboard-develop.

Did you test the PR changes with a react app with web3-onboard installed?

  • I ran more testing by trying to integrate your AppState & useSyncExternalStore version with a separate react app using npm i @web3-onboard/react.
    I, however, ran into some issues you might want to take a look at:
    image

  • import { useSyncExternalStore } from 'use-sync-external-store' didn't work until I added the shim:
    import {useSyncExternalStore} from 'use-sync-external-store/shim';

Thanks.

@iamacook
Copy link
Author

Hi @mahmud-bn, thanks for testing!

I didn't extensively cover legacy React versions so I missed the shim issue. I've now realigned my code with the currently deployed version of @web3-onboard/react and adjusted the shim import accordingly.

I've also tested my changes in a fresh React project which included the following @web3-onboard packages:

  • @web3-onboard/core (2.2.8)
  • @web3-onboard/injected-wallets (2.0.6)
  • @web3-onboard/walletconnect (2.0.1)
  • @web3-onboard/react (2.1.6)

After builing my PR and replacing @web3-onboard/react with my it, I have (successfully) tested my it with React versions:

  • 16.8.6
  • 17.0.2
  • 18.1.0 (with Node polyfills)

I hope all is now working correctly.

@Adamj1232 Adamj1232 requested a review from mahmud-bn June 6, 2022 23:16
@mahmud-bn mahmud-bn merged commit 793d8da into thirdweb-dev:v2-web3-onboard-develop Jun 6, 2022
@mahmud-bn
Copy link
Contributor

Hi @iamacook, this has been merged. Thanks for making this PR and making subsequent changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants