-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Add flag to not transpile dynamic import()
when module is CommonJS
#43329
Comments
Just ran into this myself and noticed that this breaks ESM interop in node. Per node's documentation it is possible to load esm files from commonjs via |
@weswigham any additional context you might provide here? |
I can as I've been asked about this in a few contexts. Today we assume that all requires and imports have the same end result post-transpilation, as either commonjs or esmodules. However, it's a bit more nuanced because node supports the same keywords working differently depending on if you're in an ESM context or CommonJS context. So in a commonjs context, you normally can't use // index.cjs
let module = await import('/modules/my-module.js'); What TypeScript thinks today is that this import should be switched to a const a = Promise.resolve().then(() => __importStar(require("/modules/my-module.js"))); Which is what you want for existing cases because that was a normal feature of What's tricky is that TypeScript has no way to disambiguate whether you want This seems to be hitting a few big projects because people want to have config files in ESM but let the app stay in cjs. |
Greets... Thanks @dummdidumm for filing this issue. I was about to post the same thing about a week ago. @orta not transpiling dynamic import is an important aspect to enable for Node v12+ in regard to loading ESM for CommonJS targeted TS efforts. It will be used for a lot more than loading configuration files. I am an outside contributor The least worst workaround I could come up with is the following: As an outside contributor I certainly could not touch the build process, so the above seems to do the trick until a flag can be added to Typescript which is certainly desirable. |
If node style module resolution is enabled, and you encounter a bare module specifier in a dynamic import, would it make sense to check the imported package's |
If you also checked the extension of the file being imported for ".mjs", then yes I think that would be appropriate. This has become a big problem for using some packages in a CommonJS app that are MJS only. |
Reuse dynamicImport workaround by @TomerAberbach due to microsoft/TypeScript#43329 for escape-string-regexp as well
The beauty of // index.cjs
const one = await import('./new.mjs');
const two = await import('./old.cjs');
const three = await import('./unknown.js'); This is highly desirable to maintain interop as the ecosystem slowly moves towards ESM because I can import a file without knowing the module system. The current all-or-nothing approach is too painful.
I would be okay with either solution, but the pragma would give the most flexibility if it can be configured per call site. |
This is going to be coming in natively and without a flag/pragma I expect, when more of the node ESM support starts rolling after #44501 |
For future reference: A workaround for this issue can be had by moving the Biggest drawback of this, apart from it being confusingly non-standard, is that types won't get imported. That can be worked around by manually importing the types. Complete example: import inclusion from 'inclusion';
export async function foo(): Promise<void> {
const pMap: typeof import('p-map')['default'] = (await inclusion('p-map')).default;
} |
The lit-dev-tools package is currently CommonJS, because mostly it is used for Eleventy plugins, and Eleventy doesn't support ES modules (11ty/eleventy#836). We want ES modules for this new redirect checker script, because it needs to import some ES modules, and that is difficult to do with TypeScript, because TypeScript doesn't allow emitting an actual `import` statement, which is how CommonJS -> ESM interop works (microsoft/TypeScript#43329). We also an't really have a mix of CommonJS and ESM in the same package, because the {"type": "module"} field has to be set. We could use .mjs extensions, but TypeScript won't emit those. So the simplest solution seems to be to just have two packages!
Custom script for checking lit.dev redirects. It would be nice if we could use the 3rd party link checker we already have for this somehow, but it doesn't support checking for anchors (see stevenvachon/broken-link-checker#108 -- understandable since it would require DOM parsing) which is one of the main failure cases. Fixes #467 (since we shouldn't need comments if we have the redirects checked in CI). As part of this, I created a new lit-dev-tools-esm package. The existing lit-dev-tools package is currently CommonJS, because mostly it is used for Eleventy plugins, and Eleventy doesn't support ES modules (11ty/eleventy#836). We want ES modules for this new redirect checker script, because it needs to import some ES modules, and that is difficult to do with TypeScript, because TypeScript doesn't allow emitting an actual import statement, which is how CommonJS -> ESM interop works (microsoft/TypeScript#43329). We also can't really have a mix of CommonJS and ESM in the same package, because the {"type": "module"} field has to be set to one or the other in the package.json. We could use .mjs extensions, but TypeScript won't emit those. So the simplest solution seems to be to just have two packages.
Hello everyone, has anyone else encountered a problem while using this workaround with Jest? |
The new version is an ESM package, so we need to do a dynamic import as our package is CJS. To correctly transpile the dynamic import, moduleResolution needs to be set to node16. See microsoft/TypeScript#43329
The new version is an ESM package, so we need to do a dynamic import as our package is CJS. To correctly transpile the dynamic import, moduleResolution needs to be set to node16. See microsoft/TypeScript#43329
The new version is an ESM package, so we need to do a dynamic import as our package is CJS. To correctly transpile the dynamic import, moduleResolution needs to be set to node16. See microsoft/TypeScript#43329
The new version is an ESM package, so we need to do a dynamic import as our package is CJS. To correctly transpile the dynamic import, moduleResolution needs to be set to node16. See microsoft/TypeScript#43329
The new version is an ESM package, so we need to do a dynamic import as our package is CJS. To correctly transpile the dynamic import, moduleResolution needs to be set to node16. See microsoft/TypeScript#43329
The new version is an ESM package, so we need to do a dynamic import as our package is CJS. To correctly transpile the dynamic import, moduleResolution needs to be set to node16. See microsoft/TypeScript#43329
The new version is an ESM package, so we need to do a dynamic import as our package is CJS. To correctly transpile the dynamic import, moduleResolution needs to be set to node16. See microsoft/TypeScript#43329
Unfortunately, on our use case none of the proposed workarounds work, because they are incompatible with a strict Content Security Policy. We cannot add If you are asking why would I transpile to CJS even though I'm using the module in the browser: a) I'm already using webpack so I don't care about module formats, and b) Jest doesn't support ESM and I don't want my users to be messing up with Jest and babel transforms just to run some tests. Is there any workaround that doesn't require to eval code, or is there a flag planned to solve this issue? Thanks |
It is work for me after set tsconfig.json "compilerOptions": {
"moduleResolution": "node16",
} |
That doesn't work with module CommonJS:
|
Closes #608 This adds some esm-vs-commonjs "smartness" to umzug: 1. use `require` for `.cjs` migrations and `import` for `.mjs` migrations (and their typescript equivalents) 2. use `require` for `.js` migrations _if_ `typeof require.main === 'object'`, and `import` to `.js` migrations otherwise 3. use the same criteria to create (c)js vs mjs templates when creating migration files 4. add `"moduleResolution": "Node16"` to tsconfig.lib.json to make sure `import(filepath)` doesn't get transpiled into `__importStar(require(filepath))` (see [here](microsoft/TypeScript#43329 (comment)) and [here](microsoft/TypeScript#43329 (comment))) Tests/examples: - add a `vanilla-esm` example to make sure using `import` / top-level await works - add a step to the `test_pkg` job to make sure vitest isn't hiding gnarly import problems - this is installing the compiled library as a `.tgz`, and with no other dev/prod dependencies like vitest or ts-node having been installed, so should be very close to what end users will do Didn't: - add a wrapper.mjs file in the compiled folder as suggested in #608 (comment), mostly just because it didn't seem to be necessary? It seems to work fine when imported from an ES-module, using top-level await, etc., even though umzug is itself a commonjs module. <details> <summary>original body</summary> ~Related to #608 - although does not close it.~ ~This adds built-in support for `.mjs` and `.mts` files. `.mjs` should "just work" - write migrations as ESM modules and they'll be imported in the right way. For the current major version, `.js` will continue to be assumed commonjs. ESM-fans will just have to type that extra `m` in their filenames.~ ~This PR _doesn't_ add a wrapper file so that the umzug library itself can be imported as an ES module. That can be done in a follow-up PR. In the meantime, ESM users can use `createRequire` as in the [existing ESM example](https://github.com/sequelize/umzug/tree/main/examples/2.es-modules).~ </details> --------- Co-authored-by: Misha Kaletsky <mmkal@users.noreply.github.com>
I also had to enable "lib": ["ES2022"],
"target": "ES2022",
"module": "Node16",
"moduleResolution": "Node16",
// currently necessary due to ESM only imports
"skipLibCheck": true, |
Solution to a challenging caseI solved a particularly difficult case this way, by writing a module in CommonJS outside of the TypeScript source tree (so that it doesn't get compiled) that simply re-exports the desired ESM library. Here's the setup:
Here's the project tree visually (I added the .
βββ build
β βββ bin
β β βββ cli.js
β βββ src
β βββ index.js.map
β βββ index.js
+ βββ lib
+ β βββ importGlobby.d.ts
+ β βββ importGlobby.js
βββ package.json
βββ src
β βββ index.ts
βββ tsconfig.json File contents
|
I did but not exactly your case. I faced an error using the At the end, I decided to add My script line in ...
"jest": "NODE_OPTIONS=\"$NODE_OPTIONS --experimental-vm-modules\" jest ..."
... |
I'm dying of this problem. This is how he uses it import awaitImport from "await-import-dont-compile";
import { pathToFileURL } from "url";
export const dynamicImport = async (filepath: string) => {
return await awaitImport(pathToFileURL(filepath).href);
}; Hope it helps!!! |
If someone else is facing this issue but doesn't want to install an additional package, this is an ugly but possible workaround: Found in this solution here: |
end of 2024 and it is still an issue. sigh... |
Suggestion
π Search Terms
List of keywords you searched for before creating this issue. Write them down here so that others can find this suggestion more easily and help provide feedback.
dynamic import, commonjs, esm, node
β Viability Checklist
My suggestion meets these guidelines:
β Suggestion
Add a tsconfig flag to not transpile dynamic imports when module is CommonJS. Something like
transpileDynamicImport
which would betrue
by default and only take effect when"module": "CommonJS"
.π Motivating Example
Since Node 12, it is possible to use dynamic imports. Yet I'm not able to tell the TS compiler to not transpile this into
Promise.resolve().then(() => __importStar(require('..')));
. This prevents users from importing ES modules into CommonJS, which will become increasingly common now that Node is transitioning to ES modules.π» Use Cases
The main use case is to be able to import ES modules into CommonJS, which is possible to do with
import()
. The workaround today involves an additional build step which does replacements to hide theimport()
statement from TS so it does not get transpiled and re-add it later on, which is suboptimal.The text was updated successfully, but these errors were encountered: