-
Notifications
You must be signed in to change notification settings - Fork 585
BREAKING: Remove deprecated Browserify options from CLI #3313
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3313 +/- ##
==========================================
+ Coverage 94.73% 97.79% +3.06%
==========================================
Files 518 363 -155
Lines 11947 9893 -2054
Branches 1836 1613 -223
==========================================
- Hits 11318 9675 -1643
+ Misses 629 218 -411 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -7,7 +7,7 @@ | |||
"url": "https://github.com/MetaMask/snaps.git" | |||
}, | |||
"source": { | |||
"shasum": "HehP6oFGpT5GZKlzNTVsHCTeTOlBF1044VvfhQGOCEk=", | |||
"shasum": "6MsBR6Q/fNzDp5dGRWhBZLIBYu4o31cZRTI9iIUdIn0=", |
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.
Why did this Snap change? 🤔
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.
One of the Webpack module IDs changed, I guess because the loader changed slightly.
$ diff new.js old.js
215c215
< var _build_program_wasm__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(580);
---
> var _build_program_wasm__WEBPACK_IMPORTED_MODULE_0__ = __webpack_require__(618);
236c236
< 580: (module, __webpack_exports__, __webpack_require__) => {
---
> 618: (module, __webpack_exports__, __webpack_require__) => {
@@ -51,12 +56,3 @@ export function getFunctionLoader<Options>( | |||
}, | |||
}; | |||
} | |||
|
|||
// When running as CJS, we need to export the loader as a default export, since |
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.
Do we not still want to allow the CLI to run under CJS?
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.
This was only needed for tsup
. ts-bridge
doesn't have the same issue.
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.
Ah 👍
// Since this is an end-to-end test, and we're working with a "real" snap, | ||
// we have to make a copy of the original snap manifest, so we can modify it | ||
// and reset it after the test. | ||
originalManifest = await fs.readFile( |
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.
Why do we not need this anymore?
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.
This was only needed for Browserify, I think because the shasum changed in some cases in the tests? Running the E2E test for Webpack doesn't modify the manifest.
This removes the
bundler: 'browserify'
option and related options from the CLI. These were deprecated since July 20, 2023 (#1521), and the default bundler was changed from Browserify to Webpack in February 2024 (#2214). With this PR, Browserify and the legacy config format are no longer supported.