feat: upgrade assets-controllers to v38.2.0 #27629
feat: fix dynamic import error in .cjs
- Loading branch information
commit d106c7fe457e11c9839aeaed710c832d19c2e7aa
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,21 @@ | ||||||
| diff --git a/dist/assetsUtil.cjs b/dist/assetsUtil.cjs | ||||||
| index e90a1b6767bc8ac54b7a4d580035cf5db6861dca..cbda9aba94bff918ef2ff56466f0239ce053d441 100644 | ||||||
| --- a/dist/assetsUtil.cjs | ||||||
| +++ b/dist/assetsUtil.cjs | ||||||
| @@ -2,6 +2,7 @@ | ||||||
| var __importDefault = (this && this.__importDefault) || function (mod) { | ||||||
| return (mod && mod.__esModule) ? mod : { "default": mod }; | ||||||
| }; | ||||||
| +function _interopRequireWildcard(obj) { if (obj && obj.__esModule) { return obj; } else { var newObj = {}; if (obj != null) { for (var key in obj) { if (Object.prototype.hasOwnProperty.call(obj, key)) { newObj[key] = obj[key]; } } } newObj.default = obj; return newObj; } } | ||||||
| Object.defineProperty(exports, "__esModule", { value: true }); | ||||||
| exports.fetchTokenContractExchangeRates = exports.reduceInBatchesSerially = exports.divideIntoBatches = exports.ethersBigNumberToBN = exports.addUrlProtocolPrefix = exports.getFormattedIpfsUrl = exports.getIpfsCIDv1AndPath = exports.removeIpfsProtocolPrefix = exports.isTokenListSupportedForNetwork = exports.isTokenDetectionSupportedForNetwork = exports.SupportedTokenDetectionNetworks = exports.formatIconUrlWithProxy = exports.formatAggregatorNames = exports.hasNewCollectionFields = exports.compareNftMetadata = exports.TOKEN_PRICES_BATCH_SIZE = void 0; | ||||||
| const controller_utils_1 = require("@metamask/controller-utils"); | ||||||
| @@ -221,7 +222,7 @@ async function getIpfsCIDv1AndPath(ipfsUrl) { | ||||||
| const index = url.indexOf('/'); | ||||||
| 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"))); | ||||||
|
||||||
| + const { CID } = await Promise.resolve().then(() => _interopRequireWildcard(require("multiformats"))); | |
| + const { CID } = _interopRequireWildcard(require("multiformats")); |
Contributor
Author
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;
Contributor
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.
Contributor
Author
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 🙏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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
await import()call being left in there was intentional. Not only isawait import()available to commonjs in node v12+, passing an ESM-only module likemultiformatsintorequirealso crashes node or otherwise may cause undefined behavior.The
importcall causing failed test runs is also expected, because dynamic import is only available behind aNODE_OPTIONS=--experimental-vm-modulesflag. 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
_interopRequireWildcardis actually executing theCIDimport 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 definedin 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is running under lavamoat and dynamic import is deliberately not supported - hence a transform changing all functions named
importinto__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.
Uh oh!
There was an error while loading. Please reload this page.
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.
@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-controllersandmultiformats-- 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.requiredoesn't work, and neither does static import syntax that transpiles down torequire.This 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 intorequirecalls. This update is expected to propagate to all of our TypeScript repos.In this specific case, we can use the patch as is because
multiformatshappens to work withrequirecalls 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:
@metamask/superstruct).All of these would be major, if not prohibitive, undertakings.
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.
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.jshttps://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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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-levelawait.Based on anecdotal experience from applying
Node16to 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
superstructto@metamask/superstructended 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.