Update packages to be browser-first and use separate export for Node.js by Mrtenz · Pull Request #2211 · MetaMask/snaps

Mrtenz · 2024-02-22T10:08:28Z

This refactors all packages to be browser-first: All APIs and packages used should be compatible with browsers. This means no use of Node.js builtins, or native packages. Node-specific APIs have been moved to a separate export /node (e.g., @metamask/snaps-controllers/node). For the snaps-controllers package I've also added a /react-native export, which exports the React Native webview service.

This also means we can remove the "browser" field from the packages that were using it.

Breaking changes

Anything that uses Node.js was removed from the default export, and needs to be imported from /node.
The React Native webview service was moved, and needs to be imported from /react-native.

codecov · 2024-02-22T13:16:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (9617754) to head (0b48769).

Additional details and impacted files

@@ Coverage Diff @@## tsup #2211 +/- ##
=======================================
Coverage 96.60% 96.60% =======================================
Files 335 337 +2 Lines 7597 7599 +2 Branches 1175 1175 =======================================
+ Hits 7339 7341 +2 
Misses 258 258

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Mrtenz · 2024-02-22T13:34:38Z

+ return undefined;
+ }
+- return search.substr(star, search.length - part2.length);
+ return search.substr(star, search.length - part2.length - part1.length);


I'm not sure if this is a bug or not, but the previous behaviour was unexpected (and I'm not sure how this worked before). Basically when checking a tsconfig.json path with a wildcard, this function returns the replacement for the path.
Pattern: @metamask/*/node
Input: @metamask/snaps-utils/node
Result: snaps-utils
Pattern: @metamask/*
Input: @metamask/snaps-controllers
Result: snaps-controllers
And so on. Before this change however, it would return:
Pattern: @metamask/*/node
Input: @metamask/snaps-utils/node
Result: snaps-utils/node
Meaning that it would resolve to packages/snaps-utils/node/src/... and some other invalid paths. This causes it to fall back to Node.js' default resolver, which would look at the package.json of snaps-utils and try to load packages/snaps-utils/dist/node.js, but we can't do that since the file may not be built yet.

FrederikBolding · 2024-02-23T11:15:41Z

+ return undefined;
+ }
+- return search.substr(star, search.length - part2.length);
+ return search.substr(star, search.length - part2.length - part1.length);


We need to try and upstream this. Also we need to determine if it is a bug before merging 😓

There's a ~3.5 year old issue and PR open in tsconfig-paths to fix this. It does appear to be a bug.

Can we try opening a well-reasoned PR with tests etc that is an easy merge for the maintainers?

Sure, but do you think that should block merging this?

It would be nice to get confirmation that it's a bug, but probably not blocking

FrederikBolding · 2024-02-23T11:16:59Z

@@ -0,0 +1,10 @@
+/* eslint-disable import/export */
+
+export * from '.';


This is a nice pattern, we should let everyone in the team know about how exports will work now.

That was my plan after merging this yeah.

FrederikBolding · 2024-02-23T11:18:47Z

General question: Can we do something to prevent Node.js imports accidentally being included in browser builds?

We have an eslint-config for browser environments, but it would be quite tedious to only enable this for certain files. Do you have any good ideas for how to do this?

Not really, that's why I ask 😅
I guess we could see if we can reduce this list and use that build failing as heuristic? https://github.com/MetaMask/snaps/blob/main/packages/snaps-simulator/webpack.config.ts#L63-L81
But it's not great.

…js (#2211) This refactors all packages to be browser-first: All APIs and packages used should be compatible with browsers. This means no use of Node.js builtins, or native packages. Node-specific APIs have been moved to a separate export `/node` (e.g., `@metamask/snaps-controllers/node`). For the `snaps-controllers` package I've also added a `/react-native` export, which exports the React Native webview service. This also means we can remove the "browser" field from the packages that were using it. ## Breaking changes - Anything that uses Node.js was removed from the default export, and needs to be imported from `/node`. - The React Native webview service was moved, and needs to be imported from `/react-native`.

This swaps out the build system for `tsup`, and adds a proper ESM build to each package. In addition to that, the following changes have been made: - All packages with Node.js-specific code have been refactored to make them browser compatible. Node.js exports were moved to a separate `/node` export, i.e., `@metamask/snaps-utils/node`. - This also applies to the React Native webview service in `snaps-controllers`, which can now be imported from `@metamask/snaps-controllers/react-native`. - The `snaps-simulator` package is no longer published to NPM. We were previously using it for `snaps-jest`, but it's no longer used now. --- This pull request consists of the following pull requests: - #2120. - #2211.

…js (#2211) This refactors all packages to be browser-first: All APIs and packages used should be compatible with browsers. This means no use of Node.js builtins, or native packages. Node-specific APIs have been moved to a separate export `/node` (e.g., `@metamask/snaps-controllers/node`). For the `snaps-controllers` package I've also added a `/react-native` export, which exports the React Native webview service. This also means we can remove the "browser" field from the packages that were using it. ## Breaking changes - Anything that uses Node.js was removed from the default export, and needs to be imported from `/node`. - The React Native webview service was moved, and needs to be imported from `/react-native`.

Mrtenz force-pushed the tsup branch from a785f62 to 9617754Compare February 22, 2024 11:07

Mrtenz added 3 commits February 22, 2024 12:08

Add /node and /react-native exports to snaps-controllers
b5426b8

Add /node export to snaps-utils
253291a

Fix build errors
6ba5a4a

Mrtenz force-pushed the mrtenz/node-exports branch from 0f97130 to 6ba5a4aCompare February 22, 2024 11:08

Mrtenz added 4 commits February 22, 2024 12:11

Add module name mapper to Jest
8dcb263

Fix tests
7f93875

Update yarn.lock
a1924ee

Fix more tests
5a7c7cf

Mrtenz added 3 commits February 22, 2024 14:16

Fix lint issues
b9fc03a

Update example Jest configs
8f9d87a

Restore port configuration for some examples
6b573ef

Mrtenz commented Feb 22, 2024
View reviewed changes

Mrtenz marked this pull request as ready for review February 22, 2024 13:35

Mrtenz requested a review from a team as a code owner February 22, 2024 13:35

FrederikBolding reviewed Feb 23, 2024
View reviewed changes

Comment threadconstraints.pro

FrederikBolding reviewed Feb 23, 2024
View reviewed changes

Comment threadpackages/snaps-utils/src/node.ts

FrederikBolding reviewed Feb 23, 2024
View reviewed changes

Mrtenz added 3 commits February 23, 2024 12:30

Remove Node.js polyfills from simulator Webpack config
7b04230

Remove unused dependency
1edf6f7

Fix vendor build
0b48769

FrederikBolding approved these changes Feb 23, 2024
View reviewed changes

Mrtenz merged commit 72cfc76 into tsupFeb 23, 2024

Mrtenz deleted the mrtenz/node-exports branch February 23, 2024 11:56

Mrtenz mentioned this pull request Feb 23, 2024
BREAKING: Use tsup and refactor exports #2210
Merged

Navigation Menu

Uh oh!

Update packages to be browser-first and use separate export for Node.js - #2211

Update packages to be browser-first and use separate export for Node.js#2211
Mrtenz merged 13 commits into
tsupfrom
mrtenz/node-exports

Mrtenz commented Feb 22, 2024•
edited
Loading

Uh oh!

codecovBot commented Feb 22, 2024•
edited
Loading

Uh oh!

MrtenzFeb 22, 2024

Uh oh!

Uh oh!

Uh oh!

FrederikBoldingFeb 23, 2024

Uh oh!

MrtenzFeb 23, 2024

Uh oh!

FrederikBoldingFeb 23, 2024

Uh oh!

MrtenzFeb 23, 2024

Uh oh!

FrederikBoldingFeb 23, 2024

Uh oh!

FrederikBoldingFeb 23, 2024

Uh oh!

MrtenzFeb 23, 2024

Uh oh!

FrederikBoldingFeb 23, 2024

Uh oh!

MrtenzFeb 23, 2024

Uh oh!

FrederikBoldingFeb 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,10 @@
		/* eslint-disable import/export */

		export * from '.';

Uh oh!

Conversation

Mrtenz commented Feb 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking changes

Uh oh!

codecovBot commented Feb 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Mrtenz commented Feb 22, 2024•
edited
Loading

codecovBot commented Feb 22, 2024•
edited
Loading