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

Set webpack module type to "javascript/esm" when TS impliedNodeFormat is ESNext #1614

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

andrewbranch
Copy link
Contributor

Or in other words, make Webpack give the same special treatment to .mts files as it does .mjs files. (Same for .ts in scope of a package.json "type": "module".) In short, Webpack changes its ESM/CJS module interop strategy to match Node’s when it sees these file extensions. In order for TypeScript to accurately type check this behavior, it needs to be consistent between JS and TS files. See webpack/webpack#17288 and https://andrewbranch.github.io/interop-test/#synthesizing-default-exports-for-cjs-modules for more of an explanation. (This PR will give Webpack + ts-loader + "module": "nodenext" a 💙🌟 in the linked table.)

This is technically a breaking change, but it only affects users who are using node16/nodenext for their moduleResolution, which I suspect is rare in combination with Webpack. In the near future, I’m planning to make a new module or other TypeScript option that enables this Node-like interop behavior in .mts/.mjs files, but can be used with moduleResolution: "bundler". (This is tracked by microsoft/TypeScript#54102.)

I think this should stay open until microsoft/TypeScript#54102 has a clear resolution; I just wanted to make sure it was doable before moving on. Thanks @alexander-akait for the pointers in webpack/webpack#17288.

src/index.ts Outdated Show resolved Hide resolved
loaderContext,
instance.program || instance.languageService?.getProgram() || instance.builderProgram?.getProgram()
);
if (impliedNodeFormat === /*ts.ModuleKind.ESNext*/ 99) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"I got 99 problems but EcmaScript modules ain't one"

if (loaderOptions.transpileOnly) {
(transformers ??= {}).before = [
getSetImpliedNodeFormatTransformer(instance, loaderContext, getProgram),
...(transformers?.before ?? []),
Copy link
Member

@johnnyreilly johnnyreilly Jun 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this reminds me, I wonder whether we should bump the target for the emitted JavaScript of ts-loader at some point. I suspect it's quite an old version. "target": "es2018", - not too old



// make a (sync) resolver that follows webpack's rules
const resolveSync = makeResolver(loader._compiler!.options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need this? Based on logic it is just an error - https://github.com/TypeStrong/ts-loader/blob/main/src/resolver.ts#L11

Webpack has great API for resolving, you can use (just an example of code, we should have two resolvers for cjs and esm in the real life):

const resolve = loaderContext.getResolve({
    dependencyType: "typescript",
    conditionNames: ["types", "..."],
    mainFiles: ["index", "..."],
    extensions: [".ts", ".cts", "..."],
});

And hide a lot of options for basic resolve under the hood - less options better DX, but yeah, I think it should be done in another PR, here is just about ESM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just moved code, not code I wrote

Copy link
Contributor

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some notes about code

@johnnyreilly
Copy link
Member

This looks generally good - and it looks like @alexander-akait is providing some excellent feedback. I realise I should sort out upgrading the ts-loader test pack to 5.1 as well; will sort that in the next week sometime.

{"program":{"fileNames":["../../../node_modules/typescript/lib/lib.d.ts","../../../node_modules/typescript/lib/lib.es5.d.ts","../../../node_modules/typescript/lib/lib.dom.d.ts","../../../node_modules/typescript/lib/lib.webworker.importscripts.d.ts","../../../node_modules/typescript/lib/lib.scripthost.d.ts","../../../node_modules/typescript/lib/lib.decorators.d.ts","../../../node_modules/typescript/lib/lib.decorators.legacy.d.ts","./foo.ts","./index.ts"],"fileInfos":["a7297ff837fcdf174a9524925966429eb8e5feecc2cc55cc06574e6b092c1eaa",{"version":"6a6b471e7e43e15ef6f8fe617a22ce4ecb0e34efa6c3dfcfe7cebd392bcca9d2","affectsGlobalScope":true},{"version":"fcd3ecc9f764f06f4d5c467677f4f117f6abf49dee6716283aa204ff1162498b","affectsGlobalScope":true},{"version":"c5c5565225fce2ede835725a92a28ece149f83542aa4866cfb10290bff7b8996","affectsGlobalScope":true},{"version":"7d2dbc2a0250400af0809b0ad5f84686e84c73526de931f84560e483eb16b03c","affectsGlobalScope":true},{"version":"189c0703923150aa30673fa3de411346d727cc44a11c75d05d7cf9ef095daa22","affectsGlobalScope":true},{"version":"782dec38049b92d4e85c1585fbea5474a219c6984a35b004963b00beb1aab538","affectsGlobalScope":true},"a43230ea8da8a5ab3adc7b12f9eb9d65d1d1e5c87896fb2d8747a1a3f7a3f759","582b90393f0a99a0e2da27ccff010fe0b914246cc25e49da7e760543b0789cf8"],"root":[8,9],"options":{"composite":true,"newLine":1,"skipLibCheck":true},"fileIdsList":[[8]],"referencedMap":[[9,1]],"exportedModulesMap":[[9,1]],"semanticDiagnosticsPerFile":[8,9,1,6,7,3,2,5,4],"emitSignatures":[[8,"4c57bbad758e31eeba3abc8e95e00dbac67b9581c2e7d02884ffb14c672b1520"],[9,"822618dba4b9d398326f33458039773f2c32dc8940c6134ce0b019b1ff20d068"]],"latestChangedDtsFile":"./index.d.ts"},"version":"5.0.4"}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not 100% sure, but I think the slight tsbuildinfo format change here is due to having a (no-op, in this case) custom transformer present. The change didn’t look meaningful so I wasn’t too worried about it.

import assert from "assert";
import externalLib from "../lib/externalLib.js";
assert.deepStrictEqual(externalLib, { default: {} });
console.log("PASS");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried to use an execution test for this, but I think Karma or another tool in the pipeline was interfering with what I intended to run. This is a comparison test, but the output is executable in Node, and asserts the behavior I’m implementing. Without the { "type": "module" } in the package.json (or without this PR’s changes), externalLib would be {}. But here, it’s { default: {} }. ✨

@alexander-akait
Copy link
Contributor

just idea - maybe when developer has javascript/dynamic (i.e. define type: "javascript/dynamic") and you want to say javascript/esm we should print a warking, for example like "Typescript emits ES modules, but you handle it as regular Javascript, please remove type: "javascript/dynamic" or set it to javascript/esm" (we have loaderContext.emitWarning(new Error(message)) API for this).

@andrewbranch
Copy link
Contributor Author

@alexander-akait if someone defines the type in their loader config, which value gets used? The one I set in this PR or the loader config value?

@alexander-akait
Copy link
Contributor

@andrewbranch Your value, you override it, that is why I think we need to emit a warning to catch some weird configurations/options

@johnnyreilly
Copy link
Member

Test pack migrated to 5.1 in #1616

@andrewbranch
Copy link
Contributor Author

Just wanted to drop in and say this is still on my radar; just figuring out how to expose the related config in TypeScript itself is proving difficult.

@johnnyreilly
Copy link
Member

Merry Christmas @andrewbranch 🎄⛄!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants