feat: upgrade assets-controllers to v38.2.0 #27629
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@metamask/[email protected], npm/@metamask/[email protected], npm/[email protected] |
|
@metamaskbot update-policies |
sahar-fehri
added
the
needs-assets-ux-review
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
|
@metamaskbot update-policies |
|
Policies updated. 🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff |
Builds ready [f856efb]
Page Load Metrics (1910 ± 105 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| - const { CID } = await import("multiformats"); | ||
| + const { CID } = await Promise.resolve().then(() => _interopRequireWildcard(require("multiformats"))); |
There was a problem hiding this comment.
The await import() call being left in there was intentional. Not only is await import() available to commonjs in node v12+, passing an ESM-only module like multiformats into require also crashes node or otherwise may cause undefined behavior.
The import call causing failed test runs is also expected, because dynamic import is only available behind a NODE_OPTIONS=--experimental-vm-modules flag. This is why we've added that to all of our core test scripts (https://github.com/MetaMask/core/blob/main/packages/assets-controllers/package.json#L44-L47). We'll probably need to do the same in extension and mobile.
I guess the question here is whether we can be sure that _interopRequireWildcard is actually executing the CID import correctly, or if it's masking broken behavior.
Another question would be how to handle similar scenarios (upstream ESM-only modules) going forward. Consistently using await import() is the only way that makes sense to me.
There was a problem hiding this comment.
@MajorLift thnx a lot for looking into this 🙏 I have added some logs with this current patch and it looks like its working fine, its able to fetch CID and display the NFT media;
Without the patch, i can see the error __import__ is not defined in console again and its not displaying the images correctly;
Thanks for referencing the related test script update on core 🙏 this is not only related to tests, correct? since im able to see the error when running the app locally?
There was a problem hiding this comment.
The browser uses only ESM, so that's a really weird error message. Maybe it's coming from selenium or some other node process? I tried adding the flag to the e2e test build scripts (#27675) but I'm still getting the same error.
Since you verified that the import is working correctly I think we can move forward with the current patch. Thanks for being on top of that.
Would be nice to figure this error out for the future though. I'll do some more digging.
There was a problem hiding this comment.
This code is running under lavamoat and dynamic import is deliberately not supported - hence a transform changing all functions named import into __import__
We currently can't have dynamic import work inside compartments. It's not feasible to intercept it the way we'd need to. Since the bundler we use doesn't support it, the dynamic import would be called directly in the browser and "multiformats" is not a URL to a script file, so it would fail with a different error even if LavaMoat transforms weren't there to prevent it.
I recommend addressing this somehow in a minor refactor of the assets controller if it's intended for use in MetaMask.
When we switch to webpack with LavaMoat webpack plugin it's possible webpack will transform the dynamic import into its own syntax if configured for that, but that's the only hope for using dynamic require I see for now. Didn't check how that behaves yet. I can get back to you later.
There was a problem hiding this comment.
@naugtur Thanks for the insight! I wasn't aware that Lavamoat did this. I'll have to look into what else it blocks.
Unfortunately this a language-level CJS/ESM interop issue that isn't specific to assets-controllers and multiformats -- it applies to any ESM-only module in our dependency tree.
Most of our modules are natively CJS. We use ESM syntax because we write in TypeScript, but we expose dual CJS/ESM builds and our manifests do not specify the "module": "type" property. Because of this, the only way for our modules to handle ESM imports in a CJS-compatible manner is with dynamic import syntax i.e. require doesn't work, and neither does static import syntax that transpiles down to require.
True CommonJS modules can require an ESM-transpiled-to-CJS module, since they’re both CommonJS at runtime. But in Node.js, require crashes if it resolves to an ES module. This means published libraries cannot migrate from transpiled modules to true ESM without breaking their CommonJS (true or transpiled) consumers:
// @Filename: node_modules/dependency/index.js
export function doSomething() { /* ... */ }
// @Filename: dependent.js
import { doSomething } from "dependency";
// ✅ Works if dependent and dependency are both transpiled
// ✅ Works if dependent and dependency are both true ESM
// ✅ Works if dependent is true ESM and dependency is transpiled
// 💥 Crashes if dependent is transpiled and dependency is true ESMThis issue was previously masked by our usage of the outdated { module: "commonjs", moduleResolution: "node10" } tsconfig options. However, the core and snaps repos recently updated to "node16" for both options, which enforces sound module resolution restrictions and therefore prohibits ESM-only modules from being passed into require calls. This update is expected to propagate to all of our TypeScript repos.
In this specific case, we can use the patch as is because multiformats happens to work with require calls without breaking things, but this is a coincidence we can't rely on for all ESM packages in general.
To preserve the ban on dynamic import syntax I see three possibilities:
- Remove all ESM-only modules from all of our dependency trees, or replace them with CJS-compatible forks (we've done this before e.g.
@metamask/superstruct). - Convert all of our modules to "true ESM".
- Start using a bundler like Webpack in all of our repos to generate builds (assuming it can be set up to generate dual builds for dependencies in a way that would sidestep this issue).
All of these would be major, if not prohibitive, undertakings.
There was a problem hiding this comment.
1 and 2 are not options we're fond of.
With the exception of: until we have a lavamoat-node that supports ESM (we're working on it) we might sometimes have to eliminate esm-only packages from builds that run under lavamoat.
We should be able to switch to webpack in the foreseeable future. LavaMoat Webpack plugin is reaching 1.0 by the end of this year and we're wery close to getting a working MetaMask build out of it. 3rdparty audit is finishing too.
Meanwhile,
Joyee has made it possible to require(esm) in Node.js
https://nodejs.org/api/esm.html#interoperability-with-commonjs
https://joyeecheung.github.io/blog/2024/03/18/require-esm-in-node-js/
So we're looking for a solution to the typescript situation that we can live with for limited time.
There was a problem hiding this comment.
Agreed that 1, 2 aren't the most ideal options. ;)
I wasn't aware of --experimental-require-module, but it looks like a great stopgap. Fingers crossed that we don't run into an ESM package with top-level await.
Based on anecdotal experience from applying Node16 to core and snaps, our usage of CJS-incompatible packages is uncommon enough that we should be able to handle them on a case-by-case basis, at least for the time being.
However, the few exceptions can have an outsized impact. Migrating from superstruct to @metamask/superstruct ended up being a massive undertaking because it required us to update and release basically all transitive dependents. Removing the imports for such a widely used package will probably present difficulties as well. Hopefully we don't come across such a scenario again any time soon.
Anyway, it's very good to know that there are fundamental fixes on the way, and thank you for your work on that! Especially excited to potentially have lavamoat-node directly applied to our upstream libraries.
| -import $cockatiel from "cockatiel"; | ||
| -const { circuitBreaker, ConsecutiveBreaker, ExponentialBackoff, handleAll, retry, wrap, CircuitState } = $cockatiel; | ||
| + | ||
| +import { circuitBreaker, ConsecutiveBreaker, ExponentialBackoff, handleAll, retry, wrap, CircuitState } from "cockatiel"; |
There was a problem hiding this comment.
The fix makes sense, but I don't understand why we're getting a default import here in the first place.
@Mrtenz Is it possible that this is an artifact from ts-bridge processing mixed type vs. value imports in a single import statement?
This is the original code:
import {
circuitBreaker,
ConsecutiveBreaker,
ExponentialBackoff,
handleAll,
type IPolicy,
retry,
wrap,
CircuitState,
} from 'cockatiel';
There was a problem hiding this comment.
This is not related to type imports, those are supported by ts-bridge. I suspect this is caused by a difference in how Node.js and Webpack handle module detection... 😞
ts-bridge detects whether a package is CommonJS or ESM following Node.js' module resolution algorithm. If the package is detected as CommonJS, it checks what exports it has (using cjs-module-lexer, used by Node.js as well). If some CommonJS exports are not detected, the import is rewritten to a default import, as this is supposed to increase compatibility.
cockatiel has an ESM version, but it's only specified in the module field, which is not used by Node.js (but is used by Webpack). The CommonJS export seems to use a syntax that is not supported by cjs-module-lexer (though I need to verify this), so the exports are not detected.
There are several options here:
- We apply this patch.
- We could do this as temporary solution to get this PR merged either way.
- We disable the
modulefield in Webpack's module resolution.- This aligns it more with Node.js module resolution, but may result in a larger bundle if many dependencies use the
modulefield, but notpackage.jsonexports.
- This aligns it more with Node.js module resolution, but may result in a larger bundle if many dependencies use the
- We submit a PR to
cockatielto use theexportsfield inpackage.jsonto allow Node.js to use the ESM version as well.- I will do this regardless, but I'm not sure how long it will take for this to be merged and released.
There was a problem hiding this comment.
Yet another CJS/ESM interop scenario to keep an eye out for. :( Really appreciate the breakdown and your looking into this. I had a couple of questions:
If some CommonJS exports are not detected, the import is rewritten to a default import, as this is supposed to increase compatibility.
According to @sahar-fehri, this causes the following error:
I had this error when doing
yarn build:storybook;
export 'default' (imported as '$cockatiel') was not found in 'cockatiel'
Is storybook an outlier here, or will we always see this error if there is no default export?
The CommonJS export seems to use a syntax that is not supported by
cjs-module-lexer
Also, for this issue, is an upstream update pretty much the only possible fix?
There was a problem hiding this comment.
I guess you could say Webpack is the outlier because it looks at the module field. This is completely valid in Node.js for example:
import $cockatiel from 'cockatiel';
const { circuitBreaker } = $cockatiel;
console.log(circuitBreaker); // [Function: circuitBreaker]Because Node.js loads the CommonJS version, but Webpack loads the ESM version.
Also, for this issue, is an upstream update pretty much the only possible fix?
I'm leaning towards configuring Webpack to stop using module for better Node.js compatibility. But for a proper fix where both are able to use ESM, without the need for a patch, it will need to be fixed upstream.
There was a problem hiding this comment.
We could apply this as patch temporarily until it's updated, rather than patching assets-controller:
There was a problem hiding this comment.
Really like the idea of patching closer to the root cause.
@sahar-fehri To clarify, we'd want to remove this section of the assets-controllers patch altogether (lines 22-36), and instead add a new patch file for cockatiel that contains the changes in the cockatiel PR #102 linked above.
The Webpack issue is really... not great. I also think configuring Webpack would be the preferable solution here. Fixing or patching every upstream package that triggers this issue doesn't seem advisable or feasible.
There was a problem hiding this comment.
Thank you guys for looking into this 🙏 🙏
Just to clarify; so its okay to keep the patch for cockatiel and merge it just temporarily; until this PR is merged?
There was a problem hiding this comment.
I just realised we need the assets-controller patch regardless, because it won't with the ESM version of cockatiel otherwise anyway. The cockatiel patch would need to be applied to the core repo, requiring a new release, etc.
So I think the easiest way forward for now is to keep this patch as it is, and hope that cockatiel gets updated quickly 😅.
There was a problem hiding this comment.
Sounds like a plan! Technically, anything downstream of assets-controllers would need to have this patch applied, but that's just our two clients so doesn't seem too terrible. In any case, it makes sense to avoid a controller release containing a patch. Hopefully the cockatiel maintainers will be responsive.
There was a problem hiding this comment.
using modules across ESM/CJS boundaries differs a lot across tools and environments too. Here's some examples: https://github.com/endojs/endo-e2e-tests/blob/main/matrix/table.md
Builds ready [14a7051]
Page Load Metrics (2048 ± 133 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| const cid = index !== -1 ? url.substring(0, index) : url; | ||
| const path = index !== -1 ? url.substring(index) : undefined; | ||
| - const { CID } = await import("multiformats"); | ||
| + const { CID } = await Promise.resolve().then(() => _interopRequireWildcard(require("multiformats"))); |
There was a problem hiding this comment.
Going back to the change itself:
The await on an empty promise before running synchronous require seems unnecessary. Does anything depend on this step being asynchronous? Doesn't seem so.
| + const { CID } = await Promise.resolve().then(() => _interopRequireWildcard(require("multiformats"))); | |
| + const { CID } = _interopRequireWildcard(require("multiformats")); |
There was a problem hiding this comment.
I think in v37 this change was introduced:
BREAKING: getIpfsCIDv1AndPath, getFormattedIpfsUrl are now async functions (MetaMask/core#3645)
This patch update matches the generated build after the V37 update;
There was a problem hiding this comment.
The await is necessary in the CJS source code since we're forced to use a dynamic import. But in the build output that we're patching we definitely don't need to both then and await the synchronous _interopRequireWildcard call.
@naugtur's suggestion looks good to me, although I'd feel better with another manual check on whether CID is sourced at runtime.
There was a problem hiding this comment.
Thank you @MajorLift and @naugtur 🙏 i have removed the empty promise; could you please approve again and we merge this 🙏
sahar-fehri
dismissed stale reviews from naugtur and pedronfigueiredo
via
c32292c
|
Builds ready [a6a230a]
Page Load Metrics (1787 ± 85 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
metamaskbot
added
the
release-12.7.0
gauthierpetetin
added
release-12.6.0
Description
Upgrade assets-controllers to v38.2.0
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist