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

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

.yarn/patches/tsconfig-paths-npm-3.14.2-90ce75420d.patch

+         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.

constraints.pro

packages/snaps-utils/src/node.ts

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

.yarn/patches/tsconfig-paths-npm-3.14.2-90ce75420d.patch

+         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

packages/snaps-utils/src/node.ts

@@ -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

packages/snaps-utils/src/node.ts

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 9617754 Compare 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 6ba5a4a Compare 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

constraints.pro Show resolved Hide resolved

FrederikBolding reviewed Feb 23, 2024

View reviewed changes

packages/snaps-utils/src/node.ts Show resolved Hide resolved

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 tsup Feb 23, 2024
142 of 143 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading

Mrtenz Feb 22, 2024

FrederikBolding Feb 23, 2024

Mrtenz Feb 23, 2024

FrederikBolding Feb 23, 2024

Mrtenz Feb 23, 2024

FrederikBolding Feb 23, 2024

FrederikBolding Feb 23, 2024

Mrtenz Feb 23, 2024

FrederikBolding Feb 23, 2024

Mrtenz Feb 23, 2024

FrederikBolding Feb 23, 2024

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

		export * from '.';

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

Conversation

Mrtenz commented Feb 22, 2024 • edited Loading

Breaking changes

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

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

Mrtenz commented Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading