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 Support for Resolving .cjs, .mjs, .cts and .mts Files #1503

Merged
merged 15 commits into from Sep 19, 2022
Merged

Add Support for Resolving .cjs, .mjs, .cts and .mts Files #1503

merged 15 commits into from Sep 19, 2022

Conversation

manuth
Copy link
Contributor

@manuth manuth commented Sep 18, 2022

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 using import and .cjs files using require (introduced in 75a4dfd).

Also, I have noticed that the enhanced-resolver call is broken. Passing undefined as the first argument throws an error. Thus, the use of the enhanced-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.

@johnnyreilly
Copy link
Member

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

Also, I have noticed that the enhanced-resolver call is broken. Passing undefined as the first argument throws an error. Thus, the use of the enhanced-resolver has not been working up until this point.

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

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?

@manuth
Copy link
Contributor Author

manuth commented Sep 18, 2022

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

Also, I have noticed that the enhanced-resolver call is broken. Passing undefined as the first argument throws an error. Thus, the use of the enhanced-resolver has not been working up until this point.

Interesting. If it wasn't working, didn't that cause issues? I wasn't aware of any being experienced.

If I'm interpreting the test logs right, using "resolve"-options such as "alias" has only been possible using transpileOnly as this would leave webpack taking care about the rest, I guess...?

With enhanced-resolve working now inside ts-loader, I assume that stuff like the alias-option now work without being forced to use transpileOnly.

Not quite sure about it, though. But it does look like enhanced-resolve not working only affected a small number of edge cases.

@johnnyreilly
Copy link
Member

image

Is the yarn.lock change necessary? The diff is too large for me to review it.

@manuth
Copy link
Contributor Author

manuth commented Sep 18, 2022

image

Is the yarn.lock change necessary? The diff is too large for me to review it.

No, I didn't mean to include this file

@johnnyreilly
Copy link
Member

Cool - could you revert that please?

@manuth
Copy link
Contributor Author

manuth commented Sep 18, 2022

I reverted the change made to yarn.lock. Also, I changed this code passage a little to ensure the signature of resolveModuleNames matches the one expected by typescript:

const resolveModuleNames: typescript.ProgramHost<typescript.BuilderProgram>['resolveModuleNames'] =
(
moduleNames,
containingFile,
_reusedNames?,
redirectedReference?,
_?,
containingSourceFile?
) => {

@johnnyreilly
Copy link
Member

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:

yarn run comparison-tests -- --save-output

Make sure you yarn before you do the regeneration to ensure that what you have installed matches the yarn.lock file.

@manuth
Copy link
Contributor Author

manuth commented Sep 19, 2022

@johnnyreilly I added a remark on how to regenerate tests to the Dockerfile:

ts-loader/Dockerfile

Lines 37 to 40 in 8dc552e

# regenerate comparison-tests with:
# docker build -t ts-loader .
# yarn build
# docker run -v $(pwd):/TypeStrong/ts-loader -it ts-loader yarn run comparison-tests --save-output

Hope that's fine

@johnnyreilly
Copy link
Member

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 package.json and CHANGELOG.md to document the new feature please?

@manuth
Copy link
Contributor Author

manuth commented Sep 19, 2022

Sure! What needs to be changed inside the package.json file?

@johnnyreilly
Copy link
Member

New version number: 9.4.0!

@manuth
Copy link
Contributor Author

manuth commented Sep 19, 2022

Done! Thanks for guiding me through this 😄

CHANGELOG.md Outdated Show resolved Hide resolved
@johnnyreilly
Copy link
Member

It's a pleasure! Thanks for contributing!

Co-authored-by: John Reilly <johnny_reilly@hotmail.com>
@manuth
Copy link
Contributor Author

manuth commented Sep 19, 2022

It's a pleasure! Thanks for contributing!

You're most welcome 😄
Glad I could help

@johnnyreilly johnnyreilly merged commit a810470 into TypeStrong:main Sep 19, 2022
@johnnyreilly
Copy link
Member

Once this pipeline runs this should be live: https://github.com/TypeStrong/ts-loader/actions/runs/3080976806

@johnnyreilly
Copy link
Member

@johnnyreilly
Copy link
Member

It looks as though this change has caused an issue #1504

@manuth would you be able to take a look please? If we can resolve this quickly that'd be great - if we can't then we'll likely have to revert 9.4.0 for now.

@manuth
Copy link
Contributor Author

manuth commented Sep 20, 2022

@johnnyreilly the issue is caused by the fact that enhanced-resolve is working now. Before my change, enhanced-resolve never actually returned a result.
Because of the settings, enhanced-resolve will resolve certain imports to their corresponding .ts file now.

As seen here:

ts-loader/src/servicesHost.ts

Lines 1292 to 1297 in a810470

return resolutionResult! === undefined ||
resolutionResult.resolvedFileName ===
tsResolutionResult.resolvedFileName ||
isJsImplementationOfTypings(resolutionResult!, tsResolutionResult)
? tsResolutionResult
: resolutionResult!;

The result of enhanced-resolve has a higher priority than the result of typescript.
I'm not quite sure what to do about it.
Instead of checking the typescript result using isJsImplementationOfTypings we could just check whether it's a type declaration using our newly introduced constant RegExp.

What are your thoughts?

@johnnyreilly
Copy link
Member

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?

@johnnyreilly
Copy link
Member

Can you also look at the issue @falsandtru raises here please @manuth?

#1505

It's possible we may need to consider rolling back 9.4.0 if it doesn't look promising

@manuth
Copy link
Contributor Author

manuth commented Sep 20, 2022

I think we need to disable enhanced-resolve again for the time being until I managed it to look into it a little further.

@johnnyreilly
Copy link
Member

Sounds like a plan

DavidAnson added a commit to DavidAnson/markdownlint that referenced this pull request Jan 17, 2023
DavidAnson added a commit to DavidAnson/markdownlint that referenced this pull request Jan 18, 2023
DavidAnson added a commit to DavidAnson/markdownlint that referenced this pull request Feb 25, 2023
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.

ts-loader with Typescript 4.5 beta: "Error: TypeScript emitted no output for .../index.mts"
2 participants