Skip to content
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

Module detection treats typescript import ... = require as ES module syntax #54514

Closed
Jamesernator opened this issue Aug 23, 2024 · 8 comments
Closed
Labels
strip-types Issues or PRs related to strip-types support

Comments

@Jamesernator
Copy link

Version

v22.7.0

Platform

Linux 6.8.0-40-generic #40~22.04.3-Ubuntu SMP PREEMPT_DYNAMIC Tue Jul 30 17:30:19 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

module

What steps will reproduce the bug?

Create a .cts file with the following contents:

// example.cts
import fs = require("fs");

console.log(fs);

and run it with node --experimental-transform-types ./example.cts.

How often does it reproduce? Is there a required condition?

This happens consistently.

What is the expected behavior? Why is that the expected behavior?

This should execute as a CommonJS module.

Despite appearances import ... = require(...) is specifically for CommonJS modules (so that CommonJS modules can also import and export types, not just values).

What do you see instead?

The code is incorrectly transformed to have ES module syntax and throws an error at runtime:

(node:67783) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/home/jamesernator/projects/playground/test.cts:3
export { };
^^^^^^

SyntaxError: Unexpected token 'export'
    at wrapSafe (node:internal/modules/cjs/loader:1469:18)
    at Module._compile (node:internal/modules/cjs/loader:1491:20)
    at Object.loadCTS [as .cts] (node:internal/modules/cjs/loader:1581:10)
    at Module.load (node:internal/modules/cjs/loader:1317:32)
    at Module._load (node:internal/modules/cjs/loader:1127:12)
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:217:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:166:5)
    at node:internal/main/run_main_module:30:49

Node.js v22.7.0

Additional information

No response

@Jamesernator Jamesernator changed the title Detect modules treats typescript import require as es module syntax Module detection treats typescript import ... = require as ES module syntax Aug 23, 2024
@marco-ippolito
Copy link
Member

marco-ippolito commented Aug 23, 2024

This syntax is not supported by node without some transformation.
cc @nodejs/typescript I think this syntax is not allowed under isolatedModules
EDIT: apparently swc can safely perform the transformation

@Jamesernator
Copy link
Author

We should document this limitation as I think cannot be fixed.

I don't see why this would be different than enum or namespace, the expected transform is trivial (this is what tsc already does):

import fs = require("fs");
export = expr;

// Transforms into 

const fs = require("fs");
module.exports = expr;

@robpalme
Copy link

robpalme commented Aug 23, 2024

It looks like SWC is auto-adding export {} which is something tsc does too to force the output to be recognised as ESM.

AFAIK that feature was added to workaround an issue with downstream processing by webpack.

Given the only consumer of the emitted JS here is the Node runtime, it has no use for this generated syntax, so we should probably just pass a flag to SWC to disable that addition. It's one of the existing (but lesser known) SWC transform options and I can't find the name just now.

@kdy1
Copy link
Member

kdy1 commented Aug 23, 2024

cc @magic-akari for visibility

There's noEmptyExport: bool. It's a part of the transform config. (same level as nativeClassProperties)

It exists for the exact reason - Deno did not need export {}

@magic-akari
Copy link

cc @magic-akari for visibility

There's noEmptyExport: bool. It's a part of the transform config. (same level as nativeClassProperties)

It exists for the exact reason - Deno did not need export {}

Is this a bug in SWC?

Previously, in SWC, subsequent processing would either convert to CJS, removing export {}.
Alternatively, it would proceed with the ESM process, where import = require would cause an error, same as tsc.

We didn't have a direct use case for TypeScript pass before, so we overlooked this intermediate outcome.

@ovflowd
Copy link
Member

ovflowd commented Aug 24, 2024

This is a legit “aha” moment

@ovflowd
Copy link
Member

ovflowd commented Aug 24, 2024

@marco-ippolito will amaro need to bump its swc version?

@marco-ippolito
Copy link
Member

marco-ippolito commented Aug 24, 2024

@marco-ippolito will amaro need to bump its swc version?

No, we manually set the flag to remove export {} regardless, PR is already open to update amaro v0.1.8 with the right flag #54520

RafaelGSS pushed a commit that referenced this issue Aug 30, 2024
PR-URL: #54517
Fixes: #54514
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
RafaelGSS pushed a commit that referenced this issue Aug 30, 2024
PR-URL: #54517
Fixes: #54514
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
louwers pushed a commit to louwers/node that referenced this issue Nov 2, 2024
PR-URL: nodejs#54517
Fixes: nodejs#54514
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strip-types Issues or PRs related to strip-types support
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants