Skip to content

Fix package.json auto imports for pnpm without project references #43892

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

Merged
merged 7 commits into from
May 6, 2021

Conversation

andrewbranch
Copy link
Member

Fixes #42094—read about the reason for the symptom and the rationale behind the fix there. I’m not 100% confident this is the most direct way to fix the problem; open to other ideas if they’re not super complicated.

resolvedTypeReferenceDirective = {
primary,
resolvedFileName,
originalFileName: fileName === resolvedFileName ? undefined : fileName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be originalPath just like in ResolvedModuleFull..
checkout createResolvedModuleWithFailedLookupLocations , you would want to do something similar

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it needs to be the (original casing preserved) file name instead of a lowercased Path in order to use the correct casing during module specifier resolution. Though I see originalPath is string and not Path too, so maybe I’m confused by the existing names 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is string and not Path, check in nodeModuleNameResolverWorker though it seems like it should compare paths there and not strings as there could be case difference.( but thats separate issue that @amcasey might want to look in for his realpath fix he was working on )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if it’s not going to be normalized into a Path, the property name should use fileName and not path. And if we don’t preserve the file name, I think test cases like https://github.com/microsoft/TypeScript/blob/master/tests/cases/fourslash/autoImportSymlinkCaseSensitive.ts will start having issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry i wasnt very clear in my comment.. What i was saying was that comparison needs to be between Path but value needs to be string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, just for the equality comparison. It doesn’t look like I have easy access to the current directory or getCanonicalFileName (the latter is optional on ModuleResolutionHost) and this is public API. What do you suggest doing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think you need to fix it in this PR. But module resolution host has optional getCurrentDirectory so we can add useCaseSensitiveFileNames?(): boolean and use Path if both methods exist and compare filenames otherwise.
you would also want to make sure our all internal moduleResolutionHosts have these implemented. Again dont think this is part of your PR but needs to be handled at some point for @amcasey realpath native methods to be used

@andrewbranch andrewbranch requested a review from sheetalkamat May 3, 2021 16:40
@andrewbranch andrewbranch self-assigned this May 3, 2021
@sheetalkamat
Copy link
Member

@andrewbranch you are doing change to use originalPath instead right?

@andrewbranch
Copy link
Member Author

andrewbranch commented May 3, 2021

@sheetalkamat from the thread:

I think if it’s not going to be normalized into a Path, the property name should use fileName and not path.

Since we agreed the type should be string and not Path, I was planning on keeping the name originalFileName to match. When I see a property/variable named path but its type is string, it looks to me like a mistake.

@sheetalkamat
Copy link
Member

But you still need to copy originalPath from resolved instead of having to set it ?

Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeDirectiveIsEqualTo needs to be updated to take into account this new field

@andrewbranch
Copy link
Member Author

But you still need to copy originalPath from resolved instead of having to set it ?

@sheetalkamat I looked into this and originalPath is never set on resolved in this code path. The only place it’s set is in nodeModuleNameResolverWorker which isn’t part of the call hierarchy in resolveTypeReferenceDirective. I’m sorry, I’m just not familiar enough with module resolution to understand what you’re wanting to see without a lot more details.

@sheetalkamat
Copy link
Member

looked into this and originalPath is never set on resolved in this code path. The only place it’s set is in nodeModuleNameResolverWorker which isn’t part of the call hierarchy in resolveTypeReferenceDirective. I’m sorry, I’m just not familiar enough with module resolution to understand what you’re wanting to see without a lot more details.

sorry about that , i missed the fact that realpath is called just above this and not coming from the resolved value. So your current change is right.

@andrewbranch andrewbranch requested a review from sheetalkamat May 5, 2021 22:40
Copy link
Member

@sheetalkamat sheetalkamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ideal that we have originalPath in resolvedModule and originalFileName in resolvedTypeReferenceDirective from consistency point of view but not stuck on that.

@andrewbranch
Copy link
Member Author

I can change it to originalPath and put a warning doc comment on both. My main concern is how easy it is to determine the casing/normalization of the values contained by those properties, and a doc comment should handle that.

@andrewbranch
Copy link
Member Author

Also just noticed that originalPath is @internal, so we could rename it if we ever feel like it.

@andrewbranch andrewbranch requested a review from sheetalkamat May 6, 2021 21:13
@andrewbranch andrewbranch merged commit 96c48b7 into microsoft:master May 6, 2021
@andrewbranch andrewbranch deleted the bug/42094 branch May 6, 2021 21:51
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.

AutoImportProvider doesn’t work with pnpm unless there are project references
2 participants