fix(node-resolve): Fix bug where JS import was converted to a TS import, resulting in an error when using export maps

[tjenkinson]

Rollup Plugin Name: node-resolve

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers: fixes #911, fixes #853

Description

Currently if when using export maps we resolve to a file that does not exist we don't try the remaining options. This fixes that.

We now always make the import the user wrote the first candidate, and we also only try the exports algorithm on the first candidate, and fallback to the standard node resolve algorithm for all the candidates only if the first one determines that exports are not configured.

It's specifically a problem when importing a .js file from a .ts file when you have .ts extensions configured, because for reasons I don't fully understand we first rewrite *.js imports to *.ts imports. Typescript itself added a 'feature' to do this, so we followed them.

I think it's somehow related to js modules requiring ids to end in '*.js', but not sure why typescript decided from that to allow .js and convert .ts

[guybedford]

Just to clarify I'm understanding the logic here, this PR will:

• If the file pointed to by "exports" does not exist, it "falls back" to applying the normal resolver?
• As a result pkg/x.js can resolve pkg/x.ts?
• But this fallback itself does not apply the exports mappings?

Would help to get a clear sense of it.

[tjenkinson]

If the file pointed to by "exports" does not exist, it "falls back" to applying the normal resolver?

I don't think so. If a package uses exports we will only use that resolver. By returning null from resolveId though it means the for loop in resolveImportSpecifiers will continue to run and try the other candidates where previously it would not. The idea being that resolveImportSpecifiers will always return a result that points to a real file, or null.

Previously the loop would continue for the non exports resolver (which did return null for nonexistent files), but would not continue for the exports one.

As a result pkg/x.js can resolve pkg/x.ts?

Before this PR this is already the case for the normal resolver, and think it should also be the case for when the export map resolver is used? So that the behaviour is the same, I'm still not sure why we do the convert .js to .ts thing.

But this fallback itself does not apply the exports mappings?

Not sure what you mean by this one?

[guybedford]

By returning null from resolveId though it means the for loop in resolveImportSpecifiers will continue to run and try the other candidates where previously it would not.

This is the thing that's the issue here - "exports" is designed to be an authoritative resolution system. If the file is not found it should not resolve, only unless it is a "require" resolution in which case CommonJS extension searching can apply.

For ES module resolutions exports should include the final file extension always.

Before this PR this is already the case for the normal resolver, and think it should also be the case for when the export map resolver is used?

The issue is what if the exports mapping is like "exports": { "./*": "./dist/*.js" } - we then fallback on the directory indirection which is the problem - where the not found should be correct.

[tjenkinson]

This is the thing that's the issue here - "exports" is designed to be an authoritative resolution system. If the file is not found it should not resolve, only unless it is a "require" resolution in which case CommonJS extension searching can apply.

Hmm ok. In which case maybe we should scrap the *.js => *.ts logic when using exports then?

I also think the priority order is wrong and we should be doing

importSpecifierList.push(importee);

before we push the one with the extension switch and the directory version.

[guybedford]

In which case maybe we should scrap the *.js => *.ts logic when using exports then?

Agreed this should not be happening.

[tjenkinson]

Cool. I'll update this then so that for export maps we don't loop through importSpecifierList and instead just try importee then

[tjenkinson]

@guybedford we still loop through other options on a ResolveError. Is there a reason we shouldn't bail on one of these?

[tjenkinson]

all tests pass without this now, so I think maybe this is no longer needed since the other work I did to add includeCoreModules: false to resolve

[abdonrd]

Thanks for working on this! I look forward to it!

[guybedford]

Thank you so much @tjenkinson for your work on this PR, it is really valued. Sorry I dropped the ball on this for a while will aim to be more responsive.

I've shared some more questions mostly, if you can perhaps let me know where things stand on those hopefully we can resolve them fairly quickly, also deciding which are in and out of scope for this PR as well.

[github-actions]

Your PR title doesn't match the required format. The title should be in the conventional commit (https://www.conventionalcommits.org/en/v1.0.0-beta.4/) format. e.g.

chore(plugin-name): add pr title workflow

[tjenkinson]

Awesome thanks for the review! Nice to see it got automatically published now too great work on that @shellscape