Use symlinks when looking for module names for declaration emit #24874

weswigham · 2018-06-11T22:01:49Z

Including indirect ones (ie, where the directory is linked and the module's resolution came in as anormal resolution through the parent), which previously we would have ignored, even had we used getModuleSpecifiers.

Fixes #24829

cc @rbuckton for the changes to the compiler harness to support directory/arbitrary symlinks

rbuckton · 2018-06-11T22:19:18Z

src/harness/harness.ts


            for (const line of lines) {
                const testMetaData = optionRegex.exec(line);
-                if (testMetaData) {
+                const linkMetaData = linkRegex.exec(line);


Why match this as both and option and a link? I would recommend moving the const testMetaData … line into the else branch.

rbuckton · 2018-06-11T22:22:01Z

src/harness/harness.ts

@@ -1244,6 +1247,14 @@ namespace Harness {

            const docs = inputFiles.concat(otherFiles).map(documents.TextDocument.fromTestFile);
            const fs = vfs.createFromFileSystem(IO, !useCaseSensitiveFileNames, { documents: docs, cwd: currentDirectory });
+            if (symlinks) {


If symlinks were a vfs.FileSet, you could just call fs.apply(symlinks).

rbuckton · 2018-06-11T22:23:51Z

src/harness/harness.ts


            for (const line of lines) {
                const testMetaData = optionRegex.exec(line);
-                if (testMetaData) {
+                const linkMetaData = linkRegex.exec(line);
+                if (linkMetaData) {


If symlinks was a vfs.FileSet, you could just do:

if (!symlinks) symlinks = {}; symlinks[linkMetaData[2].trim()] = new vfs.Symlink(linkMetaData[1].trim());

weswigham · 2018-06-11T23:05:14Z

@rbuckton 👍 👎?

mhegazy · 2018-06-12T18:04:32Z

src/compiler/moduleSpecifiers.ts

+        for (const path of paths) {
+            const resolved = links.get(path)!;
+            if (startsWith(target, resolved + "/")) {
+                const relative = getRelativePathFromFile(resolved + "/file.ts", target, getCanonicalFileName);


i would create a new a new getRelativePath that takes two folders.

mhegazy · 2018-06-12T18:06:20Z

src/compiler/moduleSpecifiers.ts

+        }
+        const resolvedtarget = host.getCurrentDirectory ? resolvePath(host.getCurrentDirectory(), target) : target;
+        if (options) {
+            options.push(resolvedtarget); // Since these are speculative, we also include the original resolved name as a possibility


not sure i understand why we are adding the original name?

When we find a symlinked path that we know is correct, we make a choice to ignore the original path and not present is as an option. Since the file in this case wasn't imported directly via a symlink in the compilation already, in this case I think it's more appropriate to keep it around, that way you can still get, eg ./foo as an option even though module/foo is possible.

Introduce indirect symlink lookup to specifier deriver Use fileset, move exec vfs path resolution :shakes fist: Apply files symlink relative to dirname Use directory function

mhegazy · 2018-06-12T18:31:54Z

merge conflicts

raymondfeng · 2018-06-13T20:30:55Z

@mhegazy Is the fix released with typescript@2.9.2? If so, the problem is getting even worse:

const CONTROLLER_CLASS: BindingKey<import("../../../../../../../../Users/rfeng/Projects/loopback4/loopback-next/packages/core/node_modules/@loopback/context/src/value-promise").Constructor<any>>;

Please note the relative path has two issues:

It's relative to the absolute root
It's not correctly resolved to a module dependency @loopback/context which exports the Constructor type.

weswigham · 2018-06-13T20:45:51Z

Assuming all those leading ../'s climb out of the directories I think they do, it looks like the entire leading part of that path, ../../../../../../../../Users/rfeng/Projects/loopback4/loopback-next/packages/core/node_modules/, is totally extraneous...

@raymondfeng do you have any other path-related settings enabled for your build (outDir, sourceRoot, anything)? Are there any symlinks present other than loopback-next/packages/context being linked to loopback-next/packages/core/node_modules/@loopback/context?

raymondfeng · 2018-06-13T20:51:14Z

My repo is managed by lerna and the directory layout is illustrated below:

loopback-next
  > packages
    > build (npm module @loopback/build)
    > context (npm module @loopback/context)
    > core (npm module @loopback/core)
      > node_modules
        > @loopback
          > context -> ../../../context (symbolic link to the @loopback/context module)
          > build -> ../../../build (symbolic link to the @loopback/build module)
        ...

weswigham · 2018-06-13T20:53:36Z

Right, and your TS build settings?

raymondfeng · 2018-06-13T20:55:54Z

See https://github.com/strongloop/loopback-next/blob/master/packages/core/tsconfig.build.json which extends https://github.com/strongloop/loopback-next/blob/master/packages/build/config/tsconfig.common.json

raymondfeng · 2018-06-13T21:02:52Z

The npm run build:current command for the project is basically running /Users/rfeng/Projects/loopback4/loopback-next/packages/build/node_modules/typescript/lib/tsc.js -p tsconfig.build.json --outDir /Users/rfeng/Projects/loopback4/loopback-next/packages/core/dist8 --target es2017.

weswigham requested review from rbuckton, mhegazy, sheetalkamat and a user June 11, 2018 22:01

rbuckton requested changes Jun 11, 2018

View reviewed changes

rbuckton approved these changes Jun 11, 2018

View reviewed changes

mhegazy reviewed Jun 12, 2018

View reviewed changes

mhegazy approved these changes Jun 12, 2018

View reviewed changes

fix symlink tag, support arbitrary (ie, directory) links via @link

d0c0d1c

Introduce indirect symlink lookup to specifier deriver Use fileset, move exec vfs path resolution :shakes fist: Apply files symlink relative to dirname Use directory function

weswigham force-pushed the symlink-modulename-betterness branch from e44785d to d0c0d1c Compare June 12, 2018 18:19

Merge branch 'master' into symlink-modulename-betterness

f7424b9

weswigham mentioned this pull request Jun 12, 2018

Port import type declaration updates #24909

Merged

Accept really bad baseline updates

6739029

weswigham merged commit 61fb222 into microsoft:master Jun 12, 2018

raymondfeng mentioned this pull request Jun 13, 2018

TypeScript 2.9.x incorrectly emits relative imports for symbolically linked dependencies (lerna) #24829

Closed

weswigham mentioned this pull request Jun 14, 2018

Handle combinations of rootdir and outdir when calculating paths #24941

Merged

raymondfeng mentioned this pull request Sep 9, 2018

TypeScript generates invalid import for re-exported types #26985

Closed

arichiv mentioned this pull request Jun 26, 2019

fix(ripple) broken relative imports material-components/material-components-web-react#953

Merged

arichiv mentioned this pull request Jul 8, 2019

fix(typescript) remove extraneous imports material-components/material-components-web-react#955

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use symlinks when looking for module names for declaration emit #24874

Use symlinks when looking for module names for declaration emit #24874

weswigham commented Jun 11, 2018 •

edited

Loading

rbuckton Jun 11, 2018

rbuckton Jun 11, 2018

rbuckton Jun 11, 2018

weswigham commented Jun 11, 2018

mhegazy Jun 12, 2018

mhegazy Jun 12, 2018

weswigham Jun 12, 2018

mhegazy commented Jun 12, 2018

raymondfeng commented Jun 13, 2018

weswigham commented Jun 13, 2018 •

edited

Loading

raymondfeng commented Jun 13, 2018 •

edited

Loading

weswigham commented Jun 13, 2018

raymondfeng commented Jun 13, 2018 •

edited

Loading

raymondfeng commented Jun 13, 2018 •

edited

Loading

Use symlinks when looking for module names for declaration emit #24874

Use symlinks when looking for module names for declaration emit #24874

Conversation

weswigham commented Jun 11, 2018 • edited Loading

rbuckton Jun 11, 2018

Choose a reason for hiding this comment

rbuckton Jun 11, 2018

Choose a reason for hiding this comment

rbuckton Jun 11, 2018

Choose a reason for hiding this comment

weswigham commented Jun 11, 2018

mhegazy Jun 12, 2018

Choose a reason for hiding this comment

mhegazy Jun 12, 2018

Choose a reason for hiding this comment

weswigham Jun 12, 2018

Choose a reason for hiding this comment

mhegazy commented Jun 12, 2018

raymondfeng commented Jun 13, 2018

weswigham commented Jun 13, 2018 • edited Loading

raymondfeng commented Jun 13, 2018 • edited Loading

weswigham commented Jun 13, 2018

raymondfeng commented Jun 13, 2018 • edited Loading

raymondfeng commented Jun 13, 2018 • edited Loading

weswigham commented Jun 11, 2018 •

edited

Loading

weswigham commented Jun 13, 2018 •

edited

Loading

raymondfeng commented Jun 13, 2018 •

edited

Loading

raymondfeng commented Jun 13, 2018 •

edited

Loading

raymondfeng commented Jun 13, 2018 •

edited

Loading