TypeStrong / ts-loader

TypeScript loader for webpack
https://johnnyreilly.com/ts-loader-goes-webpack-5
MIT License
3.46k stars 431 forks source link

Add Tests and Remarks Concerning the New `.cts` And `.mts` File Extensions #1508

Closed manuth closed 2 years ago

manuth commented 2 years ago

Changes made in this PR will add settings for enabling support for .cts and .mts files to both the webpack config files in the examples and the README file now contain.

This is supposed to help users to get the proper .cts and .mts experience out-of-the-box.

johnnyreilly commented 2 years ago

This is great! Would you be able to contribute an execution test as well please? It's likely just a matter of copy/pasting this test:

https://github.com/TypeStrong/ts-loader/tree/main/test/execution-tests/basic

And doing some tweaking to cater for cts / mts files

manuth commented 2 years ago

@johnnyreilly in order to work with the new file extensions .cjs and .mjs, the option resolve.extensionAlias must be available. As far as I can see, this option has been added only a month ago: https://github.com/webpack/webpack/releases/tag/v5.74.0

Could I update ts-loaders webpack dependency to v5.74.0?

johnnyreilly commented 2 years ago

Do it. You'll probably need to regenerate comparison tests and update the yarn.lock file too

manuth commented 2 years ago

@johnnyreilly I had quite a little a struggle I could not find myself able to get karma to work with a package.json file containing "type": "module", so I decided to at least test all the other cases.

As you can see, according to the test case, import statements importing ESModule dependencies, CommonJS files and also CommonJS dependencies are allowed in files which are detected as ESModule files.

Files are detected as ESModule if they have an .mts (or .mjs respectively) extension or are part of a package with its "type" set to "module".

Furthermore, ESModule dependencies and individual ESModule files can be import()ed from CommonJS files using a "dynamic import() call".

File with a .cts (or .cjs respectively) extension and files which are part of a package without a "type" or with its "type" set to "commonjs" are detected as CommonJS.

johnnyreilly commented 2 years ago

Looks good! Do you want to regenerate the comparison tests snapshots? With a new webpack version output will have changed somewhat

manuth commented 2 years ago

@johnnyreilly I'm done refactoring the tests. Thanks again for guiding me through 😄

manuth commented 2 years ago

@johnnyreilly The Comparison Tests don't work. I assume the new version of webpack emits

Module not found: Error: Can't resolve './dep' in '.test\replacement'
resolve './dep' in '.test\replacement'

instead of

Module not found: Error: Can't resolve './dep' in '.test/replacement'
resolve './dep' in '.test/replacement'

on Windows

johnnyreilly commented 2 years ago

That is weird. We have (slightly hacky) code in place to handle platform differences:

https://github.com/TypeStrong/ts-loader/blob/d9fcbfd577486f4c6ce6b44c3f5870ef7058a569/test/comparison-tests/create-and-execute-test.js#L274

Wonder if that isn't working

johnnyreilly commented 2 years ago

BTW you might find this to be an interesting proposal: https://github.com/microsoft/TypeScript/issues/50152

Somewhat related to the work you've been tackling here. @andrewbranch is looking specifically at how TypeScript can relate to bundlers for module resolution

andrewbranch commented 2 years ago

I can’t even begin to understand what “the proper .cts and .mts experience” is supposed to be in a bundler 😅

Once --moduleResolution hybrid lands, there should be no reason for anyone to use ts-loader with any other mode. In that mode, I’m considering disallowing .cts files altogether. Hopefully that will make things simpler for you all.

johnnyreilly commented 2 years ago

Once --moduleResolution hybrid lands, there should be no reason for anyone to use ts-loader with any other mode. In that mode, I’m considering disallowing .cts files altogether. Hopefully that will make things simpler for you all.

Fascinating! Because .cts is something that's unlikely to be used much in practice (and generally not encouraged)?

andrewbranch commented 2 years ago

Because most bundlers will violate the semantics that .cts files are supposed to have. A .cts file emits to CommonJS, which means imports are transformed into requires in the output JS when you use tsc. But bundlers with built-in TypeScript support like esbuild don’t do that transformation to require first, so the imports get resolved like imports instead of requires. (Actually, even tsc does this if you compile a .cts file with --module esnext, which deserves some seriously raised eyebrows on its own.) And we seem to be reaching a point where bundler users are happy writing imports instead of requires, so I hope to just give require no special meaning to make things simpler.

andrewbranch commented 2 years ago

But bundlers with built-in TypeScript support like esbuild don’t do that transformation to require first

I meant to add, interestingly, ts-loader + Webpack is possibly the only major bundler combination that would just get this right by default since it uses tsc’s emit. But especially given that it also depends on the tsconfig settings, it’s much simpler to say, “it’s too confusing to reason about what this means, so just don’t do it.”

johnnyreilly commented 2 years ago

I meant to add, interestingly, ts-loader + Webpack is possibly the only major bundler combination that would just get this right by default

Excellent! Totally planned it that way 😉

manuth commented 2 years ago

That is weird. We have (slightly hacky) code in place to handle platform differences:

https://github.com/TypeStrong/ts-loader/blob/d9fcbfd577486f4c6ce6b44c3f5870ef7058a569/test/comparison-tests/create-and-execute-test.js#L274

Wonder if that isn't working

Oh I see - in this case it's probably because during the replacement test, webpack emits relative paths in the error- and output messages, while this regex (assuming it does what its name says) only matches drive letters (aka. absolute paths)

Gonna have a look at it now!

manuth commented 2 years ago

@johnnyreilly Turns out it's been because of the use of the NormalModuleReplacementPlugin which has changed slightly due to the webpack update.

Should be working now 😄

johnnyreilly commented 2 years ago

Well done!