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

Possible discrepancy with --experimental-strip-types #54457

Closed
cjihrig opened this issue Aug 19, 2024 · 1 comment · Fixed by #54461
Closed

Possible discrepancy with --experimental-strip-types #54457

cjihrig opened this issue Aug 19, 2024 · 1 comment · Fixed by #54461
Labels
strip-types Issues or PRs related to strip-types support

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Aug 19, 2024

Version

main (commit 561bc87)

Platform

macOS, but should reproduce on all platforms.

Subsystem

CLI

What steps will reproduce the bug?

Create three entrypoint files:

// main.js
'use strict';
async function main() {
  await import('./dep.js');
}

main();
// main.mjs
await import('./dep.js');
// main.ts
'use strict';
async function main() {
  await import('./dep.js');
}

main();

Create two dependency files (note that these files are exactly the same, but have different file extensions):

// dep.js
'use strict';
require('node:fs');
console.log('ok');
// dep.ts
'use strict';
require('node:fs');
console.log('ok');

Run all three entrypoints:

$ ./node --experimental-strip-types main.js
(node:62972) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
ok
$ ./node --experimental-strip-types main.mjs
(node:63282) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
ok
$ ./node --experimental-strip-types main.ts
(node:63489) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
ok

Update all three entrypoint files to import dep.ts instead of dep.js, and run the files again:

$ ./node --experimental-strip-types main.js
(node:69672) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:69672) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///private/tmp/repro/dep.ts is not specified and it doesn't parse as CommonJS.
Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
To eliminate this warning, add "type": "module" to /private/tmp/package.json.
file:///private/tmp/repro/dep.ts:2
require('node:fs');
^

ReferenceError: require is not defined in ES module scope, you can use import instead
    at file:///private/tmp/repro/dep.ts:2:1
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
    at async main (/private/tmp/repro/main.js:3:3)

Node.js v23.0.0-pre
$ ./node --experimental-strip-types main.mjs
(node:71132) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:71132) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///private/tmp/repro/dep.ts is not specified and it doesn't parse as CommonJS.
Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
To eliminate this warning, add "type": "module" to /private/tmp/package.json.
file:///private/tmp/repro/dep.ts:2
require('node:fs');
^

ReferenceError: require is not defined in ES module scope, you can use import instead
    at file:///private/tmp/repro/dep.ts:2:1
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
    at async file:///private/tmp/repro/main.mjs:1:1

Node.js v23.0.0-pre
$ ./node --experimental-strip-types main.ts
(node:71240) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:71240) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///private/tmp/repro/dep.ts is not specified and it doesn't parse as CommonJS.
Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
To eliminate this warning, add "type": "module" to /private/tmp/package.json.
file:///private/tmp/repro/dep.ts:2
require('node:fs');
^

ReferenceError: require is not defined in ES module scope, you can use import instead
    at file:///private/tmp/repro/dep.ts:2:1
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
    at async main (/private/tmp/repro/main.ts:3:3)

Node.js v23.0.0-pre

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

It reproduces 100% of the time in my experience.

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

I expect both scenarios to succeed because the docs state that .ts files have their module system determined the same way as .js files.

What do you see instead?

(node:71240) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///private/tmp/repro/dep.ts is not specified and it doesn't parse as CommonJS.
Reparsing as ES module because module syntax was detected. This incurs a performance overhead.
To eliminate this warning, add "type": "module" to /private/tmp/package.json.
file:///private/tmp/repro/dep.ts:2
require('node:fs');
^

ReferenceError: require is not defined in ES module scope, you can use import instead
    at file:///private/tmp/repro/dep.ts:2:1
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
    at async main (/private/tmp/repro/main.ts:3:3)

Node.js v23.0.0-pre

Additional information

The error is also confusing because it says that dep.ts does not parse as CommonJS, even though it has the same content as dep.js, which seems to be a CommonJS file.

@RedYetiDev RedYetiDev added strip-types Issues or PRs related to strip-types support repro-exists labels Aug 19, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Aug 19, 2024

// repro.mjs
await import('./imported.ts')
// imported.ts
require('node:fs');
$ node --experimental-strip-types repro.mjs                             
(node:326497) ExperimentalWarning: Type Stripping is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:326497) [MODULE_TYPELESS_PACKAGE_JSON] Warning: file:///imported.ts parsed as an ES module because module syntax was detected; to avoid the performance penalty of syntax detection, add "type": "module" to /package.json
file:///imported.ts:1
require('node:fs');
^

ReferenceError: require is not defined in ES module scope, you can use import instead
    at file:///imported.ts:1:1
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:482:26)
    at async file:///repro.mjs:1:1

Node.js v22.6.0

Also note the following warning:

[MODULE_TYPELESS_PACKAGE_JSON] Warning: file:///imported.ts parsed as an ES module because module syntax was detected; to avoid the performance penalty of syntax detection, add "type": "module" to /package.json

If imported.ts is loaded from require, this does not occur.

marco-ippolito added a commit to marco-ippolito/node that referenced this issue Aug 26, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Aug 26, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Aug 31, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Sep 2, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Sep 12, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Sep 16, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Sep 16, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Sep 16, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Sep 17, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit to marco-ippolito/node that referenced this issue Sep 17, 2024
PR-URL: nodejs#54461
Fixes: nodejs#54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
marco-ippolito added a commit that referenced this issue Sep 18, 2024
PR-URL: #54461
Backport-PR-URL: #54566
Fixes: #54457
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Zeyu "Alex" Yang <[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.

2 participants