-
-
Notifications
You must be signed in to change notification settings - Fork 431
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 Support for Resolving .cjs
, .mjs
, .cts
and .mts
Files
#1503
Conversation
Thanks for contributing. I've approved the test run so let's see what errors out. The comparison test pack is effectively a set of snapshot tests. A failing test there means "something has changed" - that could be legitimate
Interesting. If it wasn't working, didn't that cause issues? I wasn't aware of any being experienced. |
src/instances.ts
Outdated
@@ -682,7 +682,7 @@ export function getInputFileNameFromOutput( | |||
instance: TSInstance, | |||
filePath: string | |||
): string | undefined { | |||
if (filePath.match(tsTsxRegex) && !fileExtensionIs(filePath, '.d.ts')) { | |||
if (filePath.match(tsTsxRegex) && !/\.d\.([cm]?ts|tsx)$/.test(filePath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we give this regex a name and site it in constants.ts
?
If I'm interpreting the test logs right, using "resolve"-options such as "alias" has only been possible using With Not quite sure about it, though. But it does look like |
Cool - could you revert that please? |
I reverted the change made to Lines 263 to 271 in 076e585
|
Cool, could you regenerate the snapshots please? Instructions here: https://github.com/TypeStrong/ts-loader/tree/main/test/comparison-tests#regenerating-test-data Just this will do I think:
Make sure you |
@johnnyreilly I added a remark on how to regenerate tests to the Dockerfile: Lines 37 to 40 in 8dc552e
Hope that's fine |
I'm slightly surprised by the alias test change - I wouldn't have expected different behaviour there. But I'm also not worried as I think this doesn't represent broken behaviour. I think we should roll with it, if there are issues when it reaches the outside world we can revert and look again. But hopefully that wont happen. Could you update the |
Sure! What needs to be changed inside the |
New version number: 9.4.0! |
Done! Thanks for guiding me through this 😄 |
It's a pleasure! Thanks for contributing! |
Co-authored-by: John Reilly <johnny_reilly@hotmail.com>
You're most welcome 😄 |
Once this pipeline runs this should be live: https://github.com/TypeStrong/ts-loader/actions/runs/3080976806 |
This is released in https://github.com/TypeStrong/ts-loader/releases/tag/v9.4.0 |
@johnnyreilly the issue is caused by the fact that As seen here: Lines 1292 to 1297 in a810470
The result of What are your thoughts? |
I think it's worth trying your idea first to see if it resolves things. You okay with trying that out? Should be able to test with the example raised in the issue? |
Can you also look at the issue @falsandtru raises here please @manuth? It's possible we may need to consider rolling back 9.4.0 if it doesn't look promising |
I think we need to disable enhanced-resolve again for the time being until I managed it to look into it a little further. |
Sounds like a plan |
…aScript for web browsers (adds support for .cjs, see TypeStrong/ts-loader#1503).
…aScript for web browsers (adds support for .cjs, see TypeStrong/ts-loader#1503).
…aScript for web browsers (adds support for .cjs, see TypeStrong/ts-loader#1503).
Changes made in this PR will fix #1383
Currently,
ts-loader
does not consider files with extensions like.cts
and.mts
as valid source files. Changes made in this PR change the regular expressions in order to make them match these new file extensions.Furthermore, changes made in this PR update the resolve logic to automatically resolve imports performed in
.mjs
files usingimport
and.cjs
files usingrequire
(introduced in 75a4dfd).Also, I have noticed that the
enhanced-resolver
call is broken. Passingundefined
as the first argument throws an error. Thus, the use of theenhanced-resolver
has not been working up until this point. This change has been introduced in 8dda7d3.To verify that passing
undefined
as the first argument triggers an error, you might want to have a look at these pieces of code:https://github.com/webpack/enhanced-resolve/blob/651e57863e46afeccf1946b4275d7b2339152ed1/lib/index.js#L51-L58
https://github.com/webpack/enhanced-resolve/blob/651e57863e46afeccf1946b4275d7b2339152ed1/lib/Resolver.js#L231-L237
https://github.com/webpack/enhanced-resolve/blob/651e57863e46afeccf1946b4275d7b2339152ed1/lib/Resolver.js#L260-L268
I might have to mention, that changes made in previously mentioned commit 8dda7d3 break the current tests. In some test cases, there are less errors reported than expected.