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
force-pushed
the
mrtenz/node-exports
branch
from
0f97130 to
6ba5a4a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
| return undefined; | ||
| } | ||
| - return search.substr(star, search.length - part2.length); | ||
| + return search.substr(star, search.length - part2.length - part1.length); |
There was a problem hiding this comment.
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.
| return undefined; | ||
| } | ||
| - return search.substr(star, search.length - part2.length); | ||
| + return search.substr(star, search.length - part2.length - part1.length); |
There was a problem hiding this comment.
We need to try and upstream this. Also we need to determine if it is a bug before merging 😓
There was a problem hiding this comment.
There's a ~3.5 year old issue and PR open in tsconfig-paths to fix this. It does appear to be a bug.
There was a problem hiding this comment.
Can we try opening a well-reasoned PR with tests etc that is an easy merge for the maintainers?
There was a problem hiding this comment.
Sure, but do you think that should block merging this?
There was a problem hiding this comment.
It would be nice to get confirmation that it's a bug, but probably not blocking
| @@ -0,0 +1,10 @@ | |||
| /* eslint-disable import/export */ | |||
|
|
|||
| export * from '.'; | |||
There was a problem hiding this comment.
This is a nice pattern, we should let everyone in the team know about how exports will work now.
There was a problem hiding this comment.
That was my plan after merging this yeah.
There was a problem hiding this comment.
General question: Can we do something to prevent Node.js imports accidentally being included in browser builds?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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`.
…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`.
2 participants
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 thesnaps-controllerspackage I've also added a/react-nativeexport, 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
/node./react-native.