Skip to content

Inspect all possible module paths when looking for the best one to create a specifier with #25850

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

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jul 21, 2018

This allows us to look at both the real and (potentially multiple) symlink paths available for a module and choose the one that gives the "best" specifier from among all of them (since a symlinked path isn't always the best one!).

In the case of the linked issue, we were finding a symlink from the other module that was symlinked into the project we were compiling, causing us to forgo both the real path (which would give us a nice global specifier) and the project-local symlink path (which would also yield a global specifier).

Fixes #25511.

Also fixes a bug in our test harness where symlinks and json files weren't carried over into the .d.ts validation build, which was causing some unnecessary .d.ts error baselines.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

CC @rbuckton for the vfs changes

return styles;
}

// @link: tests/cases/compiler/Folder/node_modules/styled-components -> tests/cases/compiler/Folder/monorepo/package-a/node_modules/styled-components
Copy link

Choose a reason for hiding this comment

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

It looks like some tests use @link and some use @symlink, do you know why?

Copy link
Member Author

@weswigham weswigham Jul 24, 2018

Choose a reason for hiding this comment

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

@symlink links the below file to another filepath. @link links one place (like a folder) to another - like an ntfs junction.

@link is strictly more powerful - I added it because @symlink couldn't be used to repro the issues I was seeing.

@@ -194,7 +194,12 @@ namespace ts.moduleSpecifiers {
const symlinks = mapDefined(files, sf =>
sf.resolvedModules && firstDefinedIterator(sf.resolvedModules.values(), res =>
res && res.resolvedFileName === importedFileName ? res.originalPath : undefined));
return symlinks.length === 0 ? getAllModulePathsUsingIndirectSymlinks(files, getNormalizedAbsolutePath(importedFileName, host.getCurrentDirectory ? host.getCurrentDirectory() : ""), getCanonicalFileName, host) : symlinks;
const cwd = host.getCurrentDirectory ? host.getCurrentDirectory() : "";
Copy link

Choose a reason for hiding this comment

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

I'm not sure I follow all the logic here, but there may be some duplicated effort. A few lines above this we get symlinks, then in discoverProbableSymlinks there is some nearly identical (copy-pasted?) code to do the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I know, it's not duplicated at all. Here we find files whose realpath differed from their actual path, provided those files refer to the imported file. In discoverProbableSymlinks, we crawl all files looking for common parts among all files with symlinks to find probable directory junction links. If I remember correctly, anyway.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 24, 2018

@weswigham can you also port to release-3.0

@weswigham weswigham merged commit 59854bb into microsoft:master Jul 24, 2018
@weswigham weswigham deleted the use-all-symlink-locations-for-path-betterness branch July 24, 2018 20:56
weswigham added a commit to weswigham/TypeScript that referenced this pull request Jul 24, 2018
…eate a specifier with (microsoft#25850)

* Inspect all possible specifier paths when looking for the best one

* Add missing secondary option from test
mhegazy added a commit that referenced this pull request Jul 24, 2018
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.

In certain cases TS 2.9 outputs incorrect definitions (.d.ts)
2 participants