-
Notifications
You must be signed in to change notification settings - Fork 639
chore!: Build Snaps execution environments with Webpack #3322
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.
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
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
packages/snaps-execution-environments/lavamoat/build-system/policy-override.json
Outdated
Show resolved
Hide resolved
| "@swc/core>@swc/core-linux-x64-gnu": true | ||
| } | ||
| }, | ||
| "html-webpack-plugin": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the additions in the policy override here are necessary because of how Webpack loads loaders. A future update to LavaMoat should make this obsolete.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3322 +/- ##
==========================================
- Coverage 98.03% 98.03% -0.01%
==========================================
Files 402 402
Lines 11109 11107 -2
Branches 1754 1754
==========================================
- Hits 10891 10889 -2
Misses 218 218 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e775b0c to
69b64c4
Compare
| "@metamask/json-rpc-engine": "^10.0.2", | ||
| "@metamask/object-multiplex": "^2.1.0", | ||
| "@metamask/post-message-stream": "^9.0.0", | ||
| "@metamask/post-message-stream": "^10.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@metamask/post-message-stream only had a CJS version, and since it's using @metamask/utils, it caused Webpack to use the CJS version of @metamask/utils while an ESM version was available. After bumping this to 10.0.0, we can use the ESM version of both @metamask/post-message-stream and @metamask/utils, resulting in a smaller bundle.
cf1b6a7 to
5bdeb37
Compare
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on: |
5bdeb37 to
974b038
Compare
hmalik88
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
5871aee to
355864e
Compare
8f08a47 to
6908166
Compare
|
@SocketSecurity ignore npm/[email protected] Network access ok. @SocketSecurity ignore npm/[email protected] This doesn't affect us. |
bfae935 to
59b4b0f
Compare
| "build:lavamoat:policy": "yarn build:lavamoat --writeAutoPolicy && node scripts/build.js --writeAutoPolicy", | ||
| "build:lavamoat": "lavamoat --policy lavamoat/build-system/policy.json --override lavamoat/build-system/policy-override.json ./scripts/build.js", | ||
| "build:lavamoat:policy": "yarn build:lavamoat --writeAutoPolicy && LAVAMOAT_GENERATE_POLICY=true node scripts/build.js", | ||
| "build:lavamoat:test": "NODE_ENV=test yarn build:lavamoat", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely sure how to feel about this, seems somewhat problematic that we aren't actually testing with prod builds in CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference is using tsconfig paths for testing, but not for production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah I understand. Maybe worth the tradeoff
f14dff9 to
57e6670
Compare
| inlineBundle: true, | ||
|
|
||
| scuttleGlobalThis: true, | ||
| scuttleGlobalThisExceptions: ['JSON', 'ReactNativeWebView', 'String'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to verify that this works in mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does 🎉
| 'Object', | ||
| 'postMessage', | ||
| 'Reflect', | ||
| 'Set', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set, Object and Reflect are new exceptions, do we know why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these are introduced by LavaMoat boilerplate IIRC. These are safe to add, and we may be able to remove them with a future LavaMoat update.
This is a redo of #3357, which was reverted in #3359 to avoid two breaking changes in a row. Now that #3322 is merged, we can do this again. The original PR description can be found below. --- This removes the web worker execution service and execution environment, as they were unused. We can revert this PR in the future to add them again, if needed. ## Breaking changes ### `@metamask/snaps-controllers` - Remove web worker execution service. ### `@metamask/snaps-execution-environments` - Remove web worker pool execution environment. - Remove web worker executor execution environment.
Browserify is not actively maintained and we should stop recommending it. With the removal of Browserify from #3322, we can now completely remove Browserify from the repo. `@metamask/snaps-browserify-plugin` should be deprecated on NPM after merging this.
After #3322, generating LavaMoat policies requires the build files to be present. Since the examples step requires build files as well, I've moved the build to a new job which is now a requirement for the other steps.
This updates the Snaps execution environments build to use Webpack instead of Browserify. The existing Browserify script was updated to build with Webpack. In the future we may be able to use Webpack from the CLI directly, but right now for compatibility with LavaMoat Node we need to use a custom script.
I've added a new script
build:lavamoat:testwhich builds the execution environments for testing locally or in CI. The main difference with this script is that it usestsconfigpaths, which are now disabled for the production build. Doing so results in much better tree shaking, so a smaller bundle.Breaking changes
node-processandnode-threadbundles are now exported as@metamask/snaps-execution-environments/node-processand@metamask/snaps-execution-environments/node-threadrespectively.