Skip to content

Sort the paths for module specifier by closeness to importing file path #33567

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 10 commits into from
Sep 25, 2019
39 changes: 38 additions & 1 deletion src/compiler/moduleSpecifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,18 @@ namespace ts.moduleSpecifiers {
return result;
}

function numberOfDirectorySeparators(str: string) {
const match = str.match(/\//g);
return match ? match.length : 0;
}

function comparePathsByNumberOfDirectrorySeparators(a: string, b: string) {
return compareValues(
numberOfDirectorySeparators(a),
numberOfDirectorySeparators(b)
);
}

/**
* Looks for existing imports that use symlinks to this module.
* Symlinks will be returned first so they are preferred over the real path.
Expand Down Expand Up @@ -214,7 +226,32 @@ namespace ts.moduleSpecifiers {
}
});
result.push(...targets);
return result;
if (result.length < 2) return result;

// Sort by paths closest to importing file Name directory
const allFileNames = arrayToMap(result, identity, getCanonicalFileName);
const sortedPaths: string[] = [];
for (
let directory = getDirectoryPath(toPath(importingFileName, cwd, getCanonicalFileName));
allFileNames.size !== 0;
directory = getDirectoryPath(directory)
) {
const directoryStart = ensureTrailingDirectorySeparator(directory);
let pathsInDirectory: string[] | undefined;
allFileNames.forEach((canonicalFileName, fileName) => {
if (startsWith(canonicalFileName, directoryStart)) {
(pathsInDirectory || (pathsInDirectory = [])).push(fileName);
allFileNames.delete(fileName);
}
});
if (pathsInDirectory) {
if (pathsInDirectory.length > 1) {
pathsInDirectory.sort(comparePathsByNumberOfDirectrorySeparators);
}
sortedPaths.push(...pathsInDirectory);
}
}
return sortedPaths;
}

function tryGetModuleNameFromAmbientModule(moduleSymbol: Symbol): string | undefined {
Expand Down
6 changes: 5 additions & 1 deletion src/compiler/watch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,11 @@ namespace ts {
export function listFiles(program: ProgramToEmitFilesAndReportErrors, writeFileName: (s: string) => void) {
if (program.getCompilerOptions().listFiles) {
forEach(program.getSourceFiles(), file => {
writeFileName(file.fileName);
writeFileName(
!file.redirectInfo ?
file.fileName :
`${file.fileName} -> ${file.redirectInfo.redirectTarget.fileName}`
);
});
}
}
Expand Down
1 change: 1 addition & 0 deletions src/testRunner/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
"unittests/tsbuild/transitiveReferences.ts",
"unittests/tsbuild/watchEnvironment.ts",
"unittests/tsbuild/watchMode.ts",
"unittests/tsc/declarationEmit.ts",
"unittests/tscWatch/consoleClearing.ts",
"unittests/tscWatch/emit.ts",
"unittests/tscWatch/emitAndErrorUpdates.ts",
Expand Down
136 changes: 136 additions & 0 deletions src/testRunner/unittests/tsc/declarationEmit.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
namespace ts {
describe("unittests:: tsc:: declarationEmit::", () => {
verifyTsc({
scenario: "declarationEmit",
subScenario: "when same version is referenced through source and another symlinked package",
fs: () => {
const fsaPackageJson = utils.dedent`
{
"name": "typescript-fsa",
"version": "3.0.0-beta-2"
}`;
const fsaIndex = utils.dedent`
export interface Action<Payload> {
type: string;
payload: Payload;
}
export declare type ActionCreator<Payload> = {
type: string;
(payload: Payload): Action<Payload>;
}
export interface ActionCreatorFactory {
<Payload = void>(type: string): ActionCreator<Payload>;
}
export declare function actionCreatorFactory(prefix?: string | null): ActionCreatorFactory;
export default actionCreatorFactory;`;
return loadProjectFromFiles({
"/src/plugin-two/index.d.ts": utils.dedent`
declare const _default: {
features: {
featureOne: {
actions: {
featureOne: {
(payload: {
name: string;
order: number;
}, meta?: {
[key: string]: any;
}): import("typescript-fsa").Action<{
name: string;
order: number;
}>;
};
};
path: string;
};
};
};
export default _default;`,
"/src/plugin-two/node_modules/typescript-fsa/package.json": fsaPackageJson,
"/src/plugin-two/node_modules/typescript-fsa/index.d.ts": fsaIndex,
"/src/plugin-one/tsconfig.json": utils.dedent`
{
"compilerOptions": {
"target": "es5",
"declaration": true,
},
}`,
"/src/plugin-one/index.ts": utils.dedent`
import pluginTwo from "plugin-two"; // include this to add reference to symlink`,
"/src/plugin-one/action.ts": utils.dedent`
import { actionCreatorFactory } from "typescript-fsa"; // Include version of shared lib
const action = actionCreatorFactory("somekey");
const featureOne = action<{ route: string }>("feature-one");
export const actions = { featureOne };`,
"/src/plugin-one/node_modules/typescript-fsa/package.json": fsaPackageJson,
"/src/plugin-one/node_modules/typescript-fsa/index.d.ts": fsaIndex,
"/src/plugin-one/node_modules/plugin-two": new vfs.Symlink("/src/plugin-two"),
});
},
commandLineArgs: ["-p", "src/plugin-one", "--listFiles"]
});

verifyTsc({
scenario: "declarationEmit",
subScenario: "when pkg references sibling package through indirect symlink",
fs: () => loadProjectFromFiles({
"/src/pkg1/dist/index.d.ts": utils.dedent`
export * from './types';`,
"/src/pkg1/dist/types.d.ts": utils.dedent`
export declare type A = {
id: string;
};
export declare type B = {
id: number;
};
export declare type IdType = A | B;
export declare class MetadataAccessor<T, D extends IdType = IdType> {
readonly key: string;
private constructor();
toString(): string;
static create<T, D extends IdType = IdType>(key: string): MetadataAccessor<T, D>;
}`,
"/src/pkg1/package.json": utils.dedent`
{
"name": "@raymondfeng/pkg1",
"version": "1.0.0",
"description": "",
"main": "dist/index.js",
"typings": "dist/index.d.ts"
}`,
"/src/pkg2/dist/index.d.ts": utils.dedent`
export * from './types';`,
"/src/pkg2/dist/types.d.ts": utils.dedent`
export {MetadataAccessor} from '@raymondfeng/pkg1';`,
"/src/pkg2/package.json": utils.dedent`
{
"name": "@raymondfeng/pkg2",
"version": "1.0.0",
"description": "",
"main": "dist/index.js",
"typings": "dist/index.d.ts"
}`,
"/src/pkg3/src/index.ts": utils.dedent`
export * from './keys';`,
"/src/pkg3/src/keys.ts": utils.dedent`
import {MetadataAccessor} from "@raymondfeng/pkg2";
export const ADMIN = MetadataAccessor.create<boolean>('1');`,
"/src/pkg3/tsconfig.json": utils.dedent`
{
"compilerOptions": {
"outDir": "dist",
"rootDir": "src",
"target": "es5",
"module": "commonjs",
"strict": true,
"esModuleInterop": true,
"declaration": true
}
}`,
"/src/pkg2/node_modules/@raymondfeng/pkg1": new vfs.Symlink("/src/pkg1"),
"/src/pkg3/node_modules/@raymondfeng/pkg2": new vfs.Symlink("/src/pkg2"),
}),
commandLineArgs: ["-p", "src/pkg3", "--listFiles"]
});
});
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,8 @@ Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./keys"));


//// [keys.d.ts]
import { MetadataAccessor } from "@raymondfeng/pkg2";
export declare const ADMIN: MetadataAccessor<boolean, import("../../pkg1/dist").IdType>;
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 am not sure if this is better or not..
cc: @weswigham @DanielRosenwasser @RyanCavanaugh

Copy link
Member

Choose a reason for hiding this comment

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

It's probably not... I mean, technically it's correct, but when your code is seen in a structure like this (monorepo-y), you're not supposed to access sibling packages with a relative path, since the packages won't be siblings when shipped... The error was good in this case, since it called out that your types depended on a package you didn't directly depend on, which meant you needed to fix it in some way. Like this, it's.... Eck.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out we resolve that only because of how these tests run (all the sources are included irrespective of tsconfig file) when I add test using the tsc baseline I retain the error so should be ok? appending that test as a separate commit for reference 91c66a0

Copy link
Member Author

Choose a reason for hiding this comment

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

Ping So this PR is good to go?

Copy link
Member

Choose a reason for hiding this comment

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

I guess, yeah.

//// [index.d.ts]
export * from './keys';
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//// [/lib/initial-buildOutput.txt]
/lib/tsc -p src/pkg3 --listFiles
src/pkg3/src/keys.ts(2,14): error TS2742: The inferred type of 'ADMIN' cannot be named without a reference to '@raymondfeng/pkg2/node_modules/@raymondfeng/pkg1'. This is likely not portable. A type annotation is necessary.
/lib/lib.d.ts
/src/pkg3/node_modules/@raymondfeng/pkg2/node_modules/@raymondfeng/pkg1/dist/types.d.ts
/src/pkg3/node_modules/@raymondfeng/pkg2/node_modules/@raymondfeng/pkg1/dist/index.d.ts
/src/pkg3/node_modules/@raymondfeng/pkg2/dist/types.d.ts
/src/pkg3/node_modules/@raymondfeng/pkg2/dist/index.d.ts
/src/pkg3/src/keys.ts
/src/pkg3/src/index.ts
exitCode:: 1


//// [/src/pkg3/dist/index.d.ts]
export * from './keys';


//// [/src/pkg3/dist/index.js]
"use strict";
function __export(m) {
for (var p in m) if (!exports.hasOwnProperty(p)) exports[p] = m[p];
}
Object.defineProperty(exports, "__esModule", { value: true });
__export(require("./keys"));


//// [/src/pkg3/dist/keys.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var pkg2_1 = require("@raymondfeng/pkg2");
exports.ADMIN = pkg2_1.MetadataAccessor.create('1');


Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
//// [/lib/initial-buildOutput.txt]
/lib/tsc -p src/plugin-one --listFiles
/lib/lib.d.ts
/src/plugin-one/node_modules/typescript-fsa/index.d.ts
/src/plugin-one/action.ts
/src/plugin-one/node_modules/plugin-two/node_modules/typescript-fsa/index.d.ts -> /src/plugin-one/node_modules/typescript-fsa/index.d.ts
Copy link
Member

Choose a reason for hiding this comment

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

So, I didn't notice it before, but isn't listFiles consumed by some VS MSBuild API, and isn't this chance to include mappings with -> probably going to break it?

Copy link
Member

Choose a reason for hiding this comment

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

cc ... @uniqueiniquity ?

/src/plugin-one/node_modules/plugin-two/index.d.ts
/src/plugin-one/index.ts
exitCode:: 0


//// [/src/plugin-one/action.d.ts]
export declare const actions: {
featureOne: import("typescript-fsa").ActionCreator<{
route: string;
}>;
};


//// [/src/plugin-one/action.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var typescript_fsa_1 = require("typescript-fsa"); // Include version of shared lib
var action = typescript_fsa_1.actionCreatorFactory("somekey");
var featureOne = action("feature-one");
exports.actions = { featureOne: featureOne };


//// [/src/plugin-one/index.d.ts]
export {};


//// [/src/plugin-one/index.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });