Skip to content

Get packageId for relative import within a package #21130

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
5 commits merged into from
Jan 17, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion src/compiler/moduleNameResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,9 @@ namespace ts {
}
const resolvedFromFile = loadModuleFromFile(extensions, candidate, failedLookupLocations, onlyRecordFailures, state);
if (resolvedFromFile) {
return noPackageId(resolvedFromFile);
const nm = considerPackageJson ? parseNodeModuleFromPath(resolvedFromFile.path) : undefined;
const packageId = nm && getPackageJsonInfo(nm.packageDirectory, nm.subModuleName, failedLookupLocations, /*onlyRecordFailures*/ false, state).packageId;
return withPackageId(packageId, resolvedFromFile);
}
}
if (!onlyRecordFailures) {
Expand All @@ -816,6 +818,45 @@ namespace ts {
return loadNodeModuleFromDirectory(extensions, candidate, failedLookupLocations, onlyRecordFailures, state, considerPackageJson);
}

const nodeModulesPathPart = "/node_modules/";

/**
* This will be called on the successfully resolved path from `loadModuleFromFile`.
* (Not neeeded for `loadModuleFromNodeModules` as that looks up the `package.json` as part of resolution.)
*
* packageDirectory is the directory of the package itself.
* subModuleName is the path within the package.
* For `blah/node_modules/foo/index.d.ts` this is { packageDirectory: "foo", subModuleName: "" }. (Part before "/node_modules/" is ignored.)
* For `/node_modules/foo/bar.d.ts` this is { packageDirectory: "foo", subModuleName": "bar" }.
* For `/node_modules/@types/foo/bar/index.d.ts` this is { packageDirectory: "@types/foo", subModuleName: "bar" }.
*/
function parseNodeModuleFromPath(path: string): { packageDirectory: string, subModuleName: string } | undefined {
path = normalizePath(path);
const idx = path.indexOf(nodeModulesPathPart);
Copy link
Contributor

Choose a reason for hiding this comment

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

should not this be lastIndexOf instead? you want the last node_modules in the list right?

Copy link
Author

Choose a reason for hiding this comment

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

👍

if (idx === -1) {
return undefined;
}

const indexAfterNodeModules = idx + nodeModulesPathPart.length;
let indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterNodeModules);
if (path.charCodeAt(indexAfterNodeModules) === CharacterCodes.at) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a test for scoped packages as well

indexAfterPackageName = moveToNextDirectorySeparatorIfAvailable(path, indexAfterPackageName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to get a path like /node_modules/@asdf? How should we handle such an input if so?

Copy link
Author

Choose a reason for hiding this comment

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

A path should be a complete file path like /node_modules/@asdf/pkg/index.d.ts, not just /node_modules/@asdf.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we concerned with malformed paths and packages? As a scenario, do we care if someone is mucking about and makes a malformed entry manually? I personally think tsserver shouldn't crash, and ideally we would surface a message to a user.

Do we have some way of handling situations like this gracefully?

Copy link
Author

Choose a reason for hiding this comment

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

I personally think tsserver shouldn't crash

That's just, like, your opinion, man. 😜

But we shouldn't get a malformed path here because this is only called after a successful module resolution. And even if we did, I don't see a place where this function would crash.

Copy link
Contributor

Choose a reason for hiding this comment

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

How does the method work with submodules under @types (eg: "@types/someModule/Submodule")? Or is that not a valid input here?

Copy link
Author

Choose a reason for hiding this comment

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

In that case subModuleName would be "Submodule".

}
const packageDirectory = path.slice(0, indexAfterPackageName);
const subModuleName = removeExtensionAndIndex(path.slice(indexAfterPackageName));
Copy link
Contributor

Choose a reason for hiding this comment

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

From the caller loadModuleFromNodeModulesFolder and the use of getPackageJsonInfo, it looks like subModuleName should refer to submodule within a nested module (eg: "core" in "@angular/core"). Is this correct?

Should subModuleName be "" when we are handling a module without submodules?

Copy link
Author

Choose a reason for hiding this comment

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

subModuleName is the name of a path within a package, so for @angular/core/foo.d.ts it would be "foo" and for @angular/core/index.d.ts it would be the empty string "".

return { packageDirectory, subModuleName };
}

function moveToNextDirectorySeparatorIfAvailable(path: string, prevSeparatorIndex: number): number {
const nextSeparatorIndex = path.indexOf(directorySeparator, prevSeparatorIndex);
return nextSeparatorIndex === -1 ? prevSeparatorIndex : nextSeparatorIndex;
}

function removeExtensionAndIndex(path: string): string {
const noExtension = removeFileExtension(path);
return noExtension === "index" ? "" : removeSuffix(noExtension, "/index");
Copy link
Member

Choose a reason for hiding this comment

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

Presumably, we won't reach here for (e.g.) "index.vb"?

Copy link
Author

Choose a reason for hiding this comment

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

removeFileExtension only removes the supported file extensions (and also correctly removes all of .d.ts and not just .ts. Although it's also true that fileName shouldn't have a .vb extension since we'd never resolve a module to a .vb file.

}

/* @internal */
export function directoryProbablyExists(directoryName: string, host: { directoryExists?: (directoryName: string) => boolean }): boolean {
// if host does not support 'directoryExists' assume that directory will exist
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
//// [tests/cases/compiler/duplicatePackage_relativeImportWithinPackage.ts] ////

//// [package.json]
{
"name": "foo",
"version": "1.2.3"
}

//// [index.d.ts]
export class C {
private x: number;
}

//// [index.d.ts]
import { C } from "foo";
export const o: C;

//// [use.d.ts]
import { C } from "./index";
export function use(o: C): void;

//// [index.d.ts]
export class C {
private x: number;
}

//// [package.json]
{
"name": "foo",
"version": "1.2.3"
}

//// [index.ts]
import { use } from "foo/use";
import { o } from "a";

use(o);


//// [index.js]
"use strict";
exports.__esModule = true;
var use_1 = require("foo/use");
var a_1 = require("a");
use_1.use(a_1.o);
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
=== /index.ts ===
import { use } from "foo/use";
>use : Symbol(use, Decl(index.ts, 0, 8))

import { o } from "a";
>o : Symbol(o, Decl(index.ts, 1, 8))

use(o);
>use : Symbol(use, Decl(index.ts, 0, 8))
>o : Symbol(o, Decl(index.ts, 1, 8))

=== /node_modules/a/node_modules/foo/index.d.ts ===
export class C {
>C : Symbol(C, Decl(index.d.ts, 0, 0))

private x: number;
>x : Symbol(C.x, Decl(index.d.ts, 0, 16))
}

=== /node_modules/a/index.d.ts ===
import { C } from "foo";
>C : Symbol(C, Decl(index.d.ts, 0, 8))

export const o: C;
>o : Symbol(o, Decl(index.d.ts, 1, 12))
>C : Symbol(C, Decl(index.d.ts, 0, 8))

=== /node_modules/foo/use.d.ts ===
import { C } from "./index";
>C : Symbol(C, Decl(use.d.ts, 0, 8))

export function use(o: C): void;
>use : Symbol(use, Decl(use.d.ts, 0, 28))
>o : Symbol(o, Decl(use.d.ts, 1, 20))
>C : Symbol(C, Decl(use.d.ts, 0, 8))

=== /node_modules/foo/index.d.ts ===
export class C {
>C : Symbol(C, Decl(index.d.ts, 0, 0))

private x: number;
>x : Symbol(C.x, Decl(index.d.ts, 0, 16))
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
[
"======== Resolving module 'foo/use' from '/index.ts'. ========",
"Module resolution kind is not specified, using 'NodeJs'.",
"Loading module 'foo/use' from 'node_modules' folder, target file type 'TypeScript'.",
"Found 'package.json' at '/node_modules/foo/package.json'.",
"File '/node_modules/foo/use.ts' does not exist.",
"File '/node_modules/foo/use.tsx' does not exist.",
"File '/node_modules/foo/use.d.ts' exist - use it as a name resolution result.",
"Resolving real path for '/node_modules/foo/use.d.ts', result '/node_modules/foo/use.d.ts'.",
"======== Module name 'foo/use' was successfully resolved to '/node_modules/foo/use.d.ts'. ========",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not related to this PR, but just a thought that came to me, we are not logging the packageid anywhere. we should probably to make these tests more statically verifiable

Copy link
Author

Choose a reason for hiding this comment

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

"======== Resolving module 'a' from '/index.ts'. ========",
"Module resolution kind is not specified, using 'NodeJs'.",
"Loading module 'a' from 'node_modules' folder, target file type 'TypeScript'.",
"File '/node_modules/a/package.json' does not exist.",
"File '/node_modules/a.ts' does not exist.",
"File '/node_modules/a.tsx' does not exist.",
"File '/node_modules/a.d.ts' does not exist.",
"File '/node_modules/a/index.ts' does not exist.",
"File '/node_modules/a/index.tsx' does not exist.",
"File '/node_modules/a/index.d.ts' exist - use it as a name resolution result.",
"Resolving real path for '/node_modules/a/index.d.ts', result '/node_modules/a/index.d.ts'.",
"======== Module name 'a' was successfully resolved to '/node_modules/a/index.d.ts'. ========",
"======== Resolving module './index' from '/node_modules/foo/use.d.ts'. ========",
"Module resolution kind is not specified, using 'NodeJs'.",
"Loading module as file / folder, candidate module location '/node_modules/foo/index', target file type 'TypeScript'.",
"File '/node_modules/foo/index.ts' does not exist.",
"File '/node_modules/foo/index.tsx' does not exist.",
"File '/node_modules/foo/index.d.ts' exist - use it as a name resolution result.",
"Found 'package.json' at '/node_modules/foo/package.json'.",
"======== Module name './index' was successfully resolved to '/node_modules/foo/index.d.ts'. ========",
"======== Resolving module 'foo' from '/node_modules/a/index.d.ts'. ========",
"Module resolution kind is not specified, using 'NodeJs'.",
"Loading module 'foo' from 'node_modules' folder, target file type 'TypeScript'.",
"Found 'package.json' at '/node_modules/a/node_modules/foo/package.json'.",
"File '/node_modules/a/node_modules/foo.ts' does not exist.",
"File '/node_modules/a/node_modules/foo.tsx' does not exist.",
"File '/node_modules/a/node_modules/foo.d.ts' does not exist.",
"'package.json' does not have a 'typings' field.",
"'package.json' does not have a 'types' field.",
"File '/node_modules/a/node_modules/foo/index.ts' does not exist.",
"File '/node_modules/a/node_modules/foo/index.tsx' does not exist.",
"File '/node_modules/a/node_modules/foo/index.d.ts' exist - use it as a name resolution result.",
"Resolving real path for '/node_modules/a/node_modules/foo/index.d.ts', result '/node_modules/a/node_modules/foo/index.d.ts'.",
"======== Module name 'foo' was successfully resolved to '/node_modules/a/node_modules/foo/index.d.ts'. ========"
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
=== /index.ts ===
import { use } from "foo/use";
>use : (o: C) => void

import { o } from "a";
>o : C

use(o);
>use(o) : void
>use : (o: C) => void
>o : C

=== /node_modules/a/node_modules/foo/index.d.ts ===
export class C {
>C : C

private x: number;
>x : number
}

=== /node_modules/a/index.d.ts ===
import { C } from "foo";
>C : typeof C

export const o: C;
>o : C
>C : C

=== /node_modules/foo/use.d.ts ===
import { C } from "./index";
>C : typeof C

export function use(o: C): void;
>use : (o: C) => void
>o : C
>C : C

=== /node_modules/foo/index.d.ts ===
export class C {
>C : C

private x: number;
>x : number
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// @noImplicitReferences: true
// @traceResolution: true

// @Filename: /node_modules/a/node_modules/foo/package.json
{
"name": "foo",
"version": "1.2.3"
}

// @Filename: /node_modules/a/node_modules/foo/index.d.ts
export class C {
private x: number;
}

// @Filename: /node_modules/a/index.d.ts
import { C } from "foo";
export const o: C;

// @Filename: /node_modules/foo/use.d.ts
import { C } from "./index";
export function use(o: C): void;

// @Filename: /node_modules/foo/index.d.ts
export class C {
private x: number;
}

// @Filename: /node_modules/foo/package.json
{
"name": "foo",
"version": "1.2.3"
}

// @Filename: /index.ts
import { use } from "foo/use";
import { o } from "a";

use(o);