bug: Invalid ESM build

[martines3000]

The ESM build of the library should use the .js file extension for relative imports, else it is not a valid ES module.

Reference: https://nodejs.org/api/esm.html#mandatory-file-extensions

This also causes issues (MODULE_NOT_FOUND) with tools that support ESM (like Vitest).

Adding file extensions to relative imports should also cause no issues with the CJS build of the package.

[Mrtenz]

Interesting. Does this apply to imports from folders as well? For example with a folder structure like this:

src/
├─ foo/
│  ├─ index.ts
├─ index.ts

Is src/index.ts able to import ./foo (referencing src/foo/index.ts), or does it have to import ./foo/index.js?

[martines3000]

Maybe you can achieve the src/foo/index.ts import with this: tsconfig paths. Also, you need to write the imports using the .js file extension, even though you are using typescript.

It is a little bit confusing at first, but I think this gives a solid explanation (a little bit long :slightly_smiling_face:)

[Mrtenz]

Maybe you can achieve the src/foo/index.ts import with this: tsconfig paths. Also, you need to write the imports using the .js file extension, even though you are using typescript.

It is a little bit confusing at first, but I think this gives a solid explanation (a little bit long 🙂)

Much appreciated! It seems like hybrid CJS/ES builds are more complicated than I thought. Will look more into this next week.

[martines3000]

Here is some extra information, maybe it will be of some help:

• If you need some reference implementation, I tested the fixes in this repo here: fork.
• Adding .js to relative imports produces a correct output, but the solution is not that simple. The same issue is also present in the @metamask/utils dependency.
• If you set module resolution to nodenext example, it will show you some more problems.
• If you want eslint to show errors for missing .js file extensions you need to set module resolution to nodenext and set the type in package.json to module. I am not sure how this impacts the CJS build though.

Thank you for looking into it! :rocket:

[Mrtenz]

There is a fix for this in the @metamask/utils repository (MetaMask/utils#144). When that is merged I will apply the same fix to key-tree.

[martines3000]

Thank you for looking into it so fast. I like the solution you chose, it didn't require any actual code change, only replacing the build tool and bundling everything as 1 file. Smart :+1:.

[martines3000]

Does bundling everything as 1 file impact tree-shaking? I'm not worried about the bundle size here, just curious if you maybe know or thought about that.

[Mrtenz]

I like the solution you chose, it didn't require any actual code change, only replacing the build tool and bundling everything as 1 file. Smart 👍.

Thanks! I definitely wanted to avoid having to change every import, update linting rules, and so on. I couldn't find a good way with TSC or SWC to add extensions to the imports at build time, and tsup seems to do just what we need out-of-the-box.

Does bundling everything as 1 file impact tree-shaking? I'm not worried about the bundle size here, just curious if you maybe know or thought about that.

I don't think it makes any difference (but don't quote me on that). Currently, the entire bundle is imported regardless (through the root index.js). And most bundlers, like Webpack and Rollup, are pretty good at tree-shaking ESM code (not so much at tree-shaking CJS, but that's why we provide the ESM file), especially considering that most of our libraries are free from side effects.

[davidmurdoch]

Ran into this today. Is a fix on the horizon?

[mcmire]

@Mrtenz It seems like this is addressed via https://github.com/MetaMask/key-tree/pull/157, is this true?

[davidmurdoch]

Will there be a release soon?

[Mrtenz]

I thought we released this already, but apparently not. We weren't planning to do a release before the next audit (mid April), so I have to figure out if we can release without including the latest PR.

[davidmurdoch]

Thanks for the update.

This is the only esm package in metamask-extension that requires non-standard (er, for webpack -- so we're not really "standard" anyway) import resolution.