Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: project module resolution #1421

Closed
wants to merge 5 commits into from
Closed

fix: project module resolution #1421

wants to merge 5 commits into from

Conversation

jxom
Copy link
Contributor

@jxom jxom commented Oct 6, 2024 •
edited
Loading

Upon assisting @wilsoncusack with Wagmi + importing a @wevm ESM-only library into the Coinbase Wallet SDK, I noticed that the repository wasn't set up properly for importing such packages.

What is introduced?

  • addition of type: "module" property to package.json
  • fixes NodeNext module resolution
  • migrates from Jest to Vitest
  • removes barrel files

Why?

To break it down:

  • Coinbase Wallet SDK is an ESM-only package, as evident in tsconfig.json#32
  • However, the SDK's package.json had a non-existent type: "module" (ESM flag) field, even though the package is being bundled as an ESM-only package. As the type: "module" property doesn't exist, TS "thinks" the project files are CJS modules, and as a result, we get an import error when we try to import an ESM-only module The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'.
  • When adding type: "module" – TS now recognizes that our project is ESM – however, since moduleResolution: "NodeNext", this means we need to add the .js extension to all imports (yes it is a throw-off, but justification makes sense).
  • Upon adding the type: "module" property, this broke Jest. Jest's ESM support is nearly non-existent, so I migrated from Jest to Vitest, which is a trivial migration and makes the tests run a bit faster.
  • Also removed barrel files (e.g. src/core/message/index.ts) which reduced bundle size by 10%.

Before

Bundle Size (gzipped): 33.38kB

$ yarn size
✔ Adding to empty webpack project
✔ Running JS in headless Chrome

  Size:         33.38 kB with all dependencies, minified and brotlied

After

Bundle Size (gzipped): 30.29kB

$ yarn size
✔ Adding to empty webpack project
✔ Running JS in headless Chrome

  Size:         30.29 kB with all dependencies, minified and brotlied

Next

I believe this repository can also benefit from the following quick wins:

  1. Removing dependence on Buffer in-favor of TypedArrays, so consumers don't have to install Buffer polyfills.
  2. Removing EIP712 CJS modules in favor of Viem's hashTypedData
  3. Migrating from ESLint/Prettier to Biome would dramatically decrease lint times, as well as dependencies
  4. Remove dependence on tslib to further decrease bundle size.

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Oct 6, 2024 •
edited
Loading

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2

@jxom jxom changed the title feat: modernize repo fix: project module resolution Oct 6, 2024
@spencerstock
Copy link
Contributor

Greetings Jake! We'll prioritize reviewing your work here. Thanks for helping out.

@fan-zhang-sv
Copy link
Contributor

just saw this update on extension rewrite from microsoft should we opt in to this?
microsoft/TypeScript#59767

@jxom
Copy link
Contributor Author

jxom commented Oct 7, 2024

@fan-zhang-sv – Perhaps! I don't think it is released yet though – unsure if it's worth waiting.

cb-jake
cb-jake reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@cb-jake cb-jake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jxom for the PR. Definitely some changes that are needed. I'm seeing some import errors when running the playground sdk (can be started w/yarn dev from root).
image

@cb-jake
Copy link
Contributor

cb-jake commented Oct 7, 2024 •
edited
Loading

@jxom Thanks for this PR. All in all everything looks good.

A few callouts that we should look into:

  • removing the preact pragma h from the hosted UI will likely break walletlink experience. I didn't see any alternative support for this
  • The SDK Playground is in a broken state. Looks like changing the commonjs modules file extension to cjs might be the culprit. If the consuming app is required to change their module resolution to now support the wallet-sdk this would likely be considered a breaking change requiring a major version bump.

Edit: Looks like you addressed the preact pragma. Thank you!

@jxom
Copy link
Contributor Author

jxom commented Oct 7, 2024

@cb-jake – thanks! made the changes. Although, I was unable to repro the .cjs runtime error. Can you pull the latest and try again?

If the consuming app is required to change their module resolution to now support the wallet-sdk this would likely be considered a breaking change requiring a major version bump.

This shouldn't be the case as we aren't changing any of the build configuration. :)

@cb-jake
Copy link
Contributor

cb-jake commented Oct 7, 2024

Thanks @jxom Taking a look now.

@cb-jake
Copy link
Contributor

cb-jake commented Oct 7, 2024

@jxom changes look good. Running CI now. Walletlink/SCW interactions worked as expected.

@cb-jake
Copy link
Contributor

cb-jake commented Oct 7, 2024

@jxom looks like we need to update CI. The main.yml is still referencing compile-assets.js vs compile-assets.cjs

fan-zhang-sv
fan-zhang-sv reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@fan-zhang-sv fan-zhang-sv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conducted manual testing via playground, smart wallet and walletlink connection both work as expected in major CUJs, including signUp/signIn, signMsg/Tx/Calls, etc

@jxom
Copy link
Contributor Author

jxom commented Oct 7, 2024

@cb-jake pushed!

@cb-jake
Copy link
Contributor

cb-jake commented Oct 7, 2024

Thanks @jxom! Looks like there is a merge conflict (sorry about that).

@jxom
Copy link
Contributor Author

jxom commented Oct 8, 2024

np, just resolved

@vishnumad
Copy link
Contributor

@jxom This looks great, looks like the imports in validatePreferences.ts and validatePreferences.test.ts files need to be updated.

@cb-jake
Copy link
Contributor

cb-jake commented Oct 8, 2024

@jxon thanks again for all your work on this. It looks like older version of node are missing crypto.subtle.generateKey inside our tests. We may need to apply a mock for older node versions.

@jxom
Copy link
Contributor Author

jxom commented Oct 8, 2024

Do you think we should remove the 16.x / 18.x tests? Or are there Node.js consumers of coinbase-wallet-sdk?

Note: Node 16 is now end-of-life, and Node 18 will be end-of-life ~Apr 2025.

@cb-jake
Copy link
Contributor

cb-jake commented Oct 9, 2024 •
edited
Loading

With the exception of SSR apps, theres probably no other nodejs consumers. We can safely remove support for Node 16. I'm on the fence with 18 since its still in active LTS. @fan-zhang-sv @spencerstock What are your thoughts on removing the test suites for Node 16 and 18? We can replace them with v22 and v24 (In a separate PR)

https://nodejs.org/en/about/previous-releases
image

@fan-zhang-sv
Copy link
Contributor

I'm aligned on removing Node 16, but think im also leaning towards keep supporting Node 18 til end of life, to maximize the support for our consumer.

@spencerstock
Copy link
Contributor

With the exception of SSR apps, theres probably no other nodejs consumers. We can safely remove support for Node 16. I'm on the fence with 18 since its still in active LTS. @fan-zhang-sv @spencerstock What are your thoughts on removing the test suites for Node 16 and 18? We can replace them with v22 and v24 (In a separate PR)

Clean lines should be drawn here.

  1. I’m not particularly concerned about dropping support for older Node versions, provided that the broader industry is also moving towards using these newer APIs.

  2. If we don't fully support a version of Node we should remove the suite instead of filling with a mock or polyfill so that we aren't propping up test cases

  3. Do we re-evaluate a node.js version test matrix entirely (maybe we remove it)? the SDK's functions aren't usually involved in the SSR process itself but after the page is fully loaded/hydrated.

@cb-jake what do you recommend?

@cb-jake
Copy link
Contributor

cb-jake commented Oct 9, 2024

@spencerstock As I think about it, the more inclined I am to remove test support for 16 and 18. We can update the matrix to include 22 and 24. Internally we can add in a separate PR.

@jxom I have a PR to remove support for 16 and 18. Once we land this, you can rebase and we'll get these changes landed. #1424

@cb-jake
Copy link
Contributor

cb-jake commented Oct 10, 2024

@jxom #1424 is merged. If you want to rebase. We should be able to get this PR landed. Thanks again!

@cb-jake cb-jake mentioned this pull request Oct 17, 2024
Merged
@cb-jake
Copy link
Contributor

cb-jake commented Oct 17, 2024 •
edited
Loading

I updated this PR and fixed the merge conflicts here PR. We've merged the other PR. Going to close this. Thanks again @jxom. Appreciate the work on this!

@cb-jake cb-jake closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cb-jake cb-jake cb-jake left review comments

@fan-zhang-sv fan-zhang-sv fan-zhang-sv left review comments

@spencerstock spencerstock Awaiting requested review from spencerstock

@eric-ships eric-ships Awaiting requested review from eric-ships

@vishnumad vishnumad Awaiting requested review from vishnumad

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jxom @cb-heimdall @spencerstock @fan-zhang-sv @cb-jake @vishnumad