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

Only resolve symlinks in node_modules #12020

Merged
6 commits merged into from
Nov 11, 2016
Merged

Only resolve symlinks in node_modules #12020

6 commits merged into from
Nov 11, 2016

Conversation

ghost
Copy link

@ghost ghost commented Nov 3, 2016

Fixes #10364 and fixes #9552.
This is based on #11993 and will be rebased onto master once that is merged.

Based on the issues I've seen, the best heuristic to decide whether to resolve a symlink is whether it comes from node_modules. So we now do not resolve symlinks for ordinary files. (This is a breaking change.)

The change to moduleNameResolver is simple. However, our test harness did not accurately reflect what the command line compiler really did, so I had to change how it treated symlinks. I've tested the new tests with the command line compiler as well; instructions for doing so are at the bottom of each test file.

@RyanCavanaugh
Copy link
Member

@dotnet-bot test this please

@mhegazy
Copy link
Contributor

mhegazy commented Nov 8, 2016

I thought we were not going to resolve symlinks in resolveModuleNames at all, so what is the reason for the change of strategy?

@@ -563,7 +563,8 @@ namespace ts {
trace(host, Diagnostics.Loading_module_0_from_node_modules_folder, moduleName);
}
const resolved = loadModuleFromNodeModules(extensions, moduleName, containingDirectory, failedLookupLocations, state, /*checkOneLevel*/ false);
return resolved && { resolved, isExternalLibraryImport: true };
// For node_modules lookups, get the real path so that multiple accesses to an `npm link`-ed module do not create duplicate files.
return resolved && { resolved: resolvedWithRealpath(resolved, host, traceEnabled), isExternalLibraryImport: true };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to do the same for resolveTypeReferenceDirectives as well.

if (typeof commonSourceDirectory === "undefined") {
if (options.rootDir && checkSourceFilesBelongToPath(files, options.rootDir)) {
if (commonSourceDirectory === undefined) {
const emittedFiles = filterSourceFilesInDirectory(files, isSourceFileFromExternalLibrary);
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think that changes in the way how common subpath is computed should not be the part of this PR since it is not related to original issue

Copy link
Author

Choose a reason for hiding this comment

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

Review #11993, then I'll rebase.

@devedse
Copy link

devedse commented Nov 14, 2016

Can/will this fix be included in the next release of TypeScript (2.0.10)?

@mhegazy
Copy link
Contributor

mhegazy commented Nov 14, 2016

Yes it will be included in the next release; and the next release is TypeScript 2.1.3 (and not 2.0.10).

@timocov
Copy link
Contributor

timocov commented Jun 2, 2018

I thought we were not going to resolve symlinks in resolveModuleNames at all, so what is the reason for the change of strategy?

@andy-ms whether did you remember why? Could you please try to remember the reason or what can happen if provide undefined (or path => path) as CompilerHost.realpath to ts.createProgram?

@ghost
Copy link
Author

ghost commented Jun 4, 2018

I think this was just the simplest place to do the resolving.
If the host doesn't define realPath, it's treated the same as path => path. In both cases, it means the compiler will always see a symlink as a different file from the original; since it can't resolve the symlink, it has no way of knowing they're the same file. See function realPath in moduleNameResolver.ts.

@timocov
Copy link
Contributor

timocov commented Jun 10, 2018

@andy-ms Thank you for clarification!

it has no way of knowing they're the same file

What can happen in this case except getting multiple SourceFile instances for one real file but for each symlink (and, possibly, increasing memory usage or compilation time)? How it can affect on compilation?

@ghost
Copy link
Author

ghost commented Jun 11, 2018

In that case it will be just like you had to separate files with no symlink. It could cause compile errors if the file contained a class with a private member, since a class instance coming from one file won't be assignable to the other file's class. E.g.:

foo/index.d.ts:

export class C { private x: number; }

user.ts:

import { C as C1 } from "foo";
import { C as C2 } from "other/foo";

const c: C1 = new C2(); // Error

@timocov
Copy link
Contributor

timocov commented Jun 11, 2018

Oh, I get it. Thank you!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants