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

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

Merged
merged 9 commits into from
Sep 24, 2022
Merged

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

merged 9 commits into from
Sep 24, 2022

Conversation

manuth
Copy link
Contributor

@manuth manuth commented Sep 21, 2022

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
Copy link
Member

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 manuth closed this Sep 21, 2022
@manuth manuth deleted the refactor-docs branch September 21, 2022 12:00
@manuth manuth restored the refactor-docs branch September 21, 2022 13:29
@manuth manuth reopened this Sep 21, 2022
@manuth manuth closed this Sep 21, 2022
@manuth manuth deleted the refactor-docs branch September 21, 2022 13:31
@manuth manuth restored the refactor-docs branch September 21, 2022 13:32
@manuth
Copy link
Contributor Author

manuth commented Sep 21, 2022

@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?

@manuth manuth reopened this Sep 21, 2022
@manuth manuth changed the title Add Remarks About .cts And .mts to Example Configurations Add Tests and Remarks Concerning the New .cts And .mts File Extensions Sep 21, 2022
@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 21, 2022

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

@manuth
Copy link
Contributor Author

manuth commented Sep 21, 2022

@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
Copy link
Member

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

@manuth
Copy link
Contributor Author

manuth commented Sep 22, 2022

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

@manuth
Copy link
Contributor Author

manuth commented Sep 22, 2022

@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
Copy link
Member

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

.replace(new RegExp(regexEscape(rootPathWithIncorrectWindowsSeparator), 'g'), '')

Wonder if that isn't working

@johnnyreilly
Copy link
Member

johnnyreilly commented Sep 22, 2022

BTW you might find this to be an interesting proposal: microsoft/TypeScript#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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

manuth commented Sep 23, 2022

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

.replace(new RegExp(regexEscape(rootPathWithIncorrectWindowsSeparator), 'g'), '')

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
Copy link
Contributor Author

manuth commented Sep 23, 2022

@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
Copy link
Member

Well done!

@johnnyreilly johnnyreilly merged commit e76abb0 into TypeStrong:main Sep 24, 2022
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