Skip to content

Dont use baseURL relative absolute paths in declaration emit, use absolute paths in bundle emit #26341

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.

Sign up for GitHub

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 2 commits into from
Aug 10, 2018
Merged

Dont use baseURL relative absolute paths in declaration emit, use absolute paths in bundle emit #26341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
769b928
Dont use baseURL relative absolute paths in declaration emit, use abs…
weswigham Aug 9, 2018
f58f73e
Merge branch 'master' into prefer-relative-declaration-paths
weswigham Aug 9, 2018
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
2 changes: 0 additions & 2 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3900,8 +3900,6 @@ namespace ts {
compilerOptions,
contextFile,
context.tracker.moduleResolverHost,
context.tracker.moduleResolverHost.getSourceFiles!(), // TODO: GH#18217
{ importModuleSpecifierPreference: "non-relative" },
host.redirectTargetsMap,
);
links.specifierCache = links.specifierCache || createMap();
Expand Down
16 changes: 13 additions & 3 deletions src/compiler/moduleSpecifiers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,21 @@ namespace ts.moduleSpecifiers {
compilerOptions: CompilerOptions,
importingSourceFile: SourceFile,
host: ModuleSpecifierResolutionHost,
files: ReadonlyArray<SourceFile>,
preferences: ModuleSpecifierPreferences,
redirectTargetsMap: RedirectTargetsMap,
): string {
return first(first(getModuleSpecifiers(moduleSymbol, compilerOptions, importingSourceFile, host, files, preferences, redirectTargetsMap)));
const isBundle = (compilerOptions.out || compilerOptions.outFile);
if (isBundle && host.getCommonSourceDirectory) {
// For declaration bundles, we need to generate absolute paths relative to the common source dir for imports,
// just like how the declaration emitter does for the ambient module declarations - we can easily accomplish this
// using the `baseUrl` compiler option (which we would otherwise never use in declaration emit) and a non-relative
// specifier preference
compilerOptions = {
...compilerOptions,
baseUrl: host.getCommonSourceDirectory(),
};
}
const preferences: ModuleSpecifierPreferences = { importModuleSpecifierPreference: isBundle ? "non-relative" : "relative" };
return first(first(getModuleSpecifiers(moduleSymbol, compilerOptions, importingSourceFile, host, host.getSourceFiles ? host.getSourceFiles() : [importingSourceFile], preferences, redirectTargetsMap)));
}

// For each symlink/original for a module, returns a list of ways to import that file.
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5334,6 +5334,7 @@ namespace ts {
fileExists?(path: string): boolean;
readFile?(path: string): string | undefined;
getSourceFiles?(): ReadonlyArray<SourceFile>; // Used for cached resolutions to find symlinks without traversing the fs (again)
Copy link

Choose a reason for hiding this comment

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

I think the API would be better if this and getCommonSourceDirectory weren't optional members of this interface, and were just passed in as values in the one place they're needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmmmm they're on the interface because that interface is how the declaration emitter shuffles bits of its emit host into the checker to fulfill these needs. And I don't particularly want to pass in a second host object with the kinda-nice-to-have-for-one-of-the-entry-points methods (especially since {moduleResolverHost: host, declarationModuleResolverHost: host} in declarations.ts would look like we were going mad).

Copy link

Choose a reason for hiding this comment

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

I didn't mean to add a second host for those methods, but that a host isn't necessary at all. Both methods are immediately called in the first function and not used by anything else in moduleSpecifiers.
Making the method optional seems to cover up problems, such as the fact that getCommonSourceDirectory isn't defined on TypeCheckerHost (although it might happen to be defined at runtime anyway, though possibly with a different type), and users of other functions will think they need to provide these methods even when they're unused outside of getModuleSpecifierForDeclarationFile. It also looks like host.getSourceFiles() is non-optional so testing for its existence is dead code.

Copy link
Member Author

Choose a reason for hiding this comment

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

A host is merely a group of argument methods to delegate to, no? If I need two, it's reasonable to group them into a second host object.

getCommonSourceDirectory?(): string;
}

// Note: this used to be deprecated in our public API, but is still used internally
Expand Down
50 changes: 50 additions & 0 deletions tests/baselines/reference/declarationEmitPrefersPathKindBasedOnBundling.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
//// [tests/cases/compiler/declarationEmitPrefersPathKindBasedOnBundling.ts] ////

//// [scalar.ts]
export interface Scalar {
(): string;
value: number;
}

export function scalar(value: string): Scalar {
return null as any;
}
//// [spacing.ts]
import { scalar } from '../lib/operators/scalar';

export default {
get xs() {
return scalar("14px");
}
};


//// [scalar.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
function scalar(value) {
return null;
}
exports.scalar = scalar;
//// [spacing.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
var scalar_1 = require("../lib/operators/scalar");
exports.default = {
get xs() {
return scalar_1.scalar("14px");
}
};


//// [scalar.d.ts]
export interface Scalar {
(): string;
value: number;
}
export declare function scalar(value: string): Scalar;
//// [spacing.d.ts]
declare const _default: {
readonly xs: import("../lib/operators/scalar").Scalar;
};
export default _default;
29 changes: 29 additions & 0 deletions tests/baselines/reference/declarationEmitPrefersPathKindBasedOnBundling.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
=== tests/cases/compiler/src/lib/operators/scalar.ts ===
export interface Scalar {
>Scalar : Symbol(Scalar, Decl(scalar.ts, 0, 0))

(): string;
value: number;
>value : Symbol(Scalar.value, Decl(scalar.ts, 1, 12))
}

export function scalar(value: string): Scalar {
>scalar : Symbol(scalar, Decl(scalar.ts, 3, 1))
>value : Symbol(value, Decl(scalar.ts, 5, 23))
>Scalar : Symbol(Scalar, Decl(scalar.ts, 0, 0))

return null as any;
}
=== tests/cases/compiler/src/settings/spacing.ts ===
import { scalar } from '../lib/operators/scalar';
>scalar : Symbol(scalar, Decl(spacing.ts, 0, 8))

export default {
get xs() {
>xs : Symbol(xs, Decl(spacing.ts, 2, 16))

return scalar("14px");
>scalar : Symbol(scalar, Decl(spacing.ts, 0, 8))
}
};

32 changes: 32 additions & 0 deletions tests/baselines/reference/declarationEmitPrefersPathKindBasedOnBundling.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
=== tests/cases/compiler/src/lib/operators/scalar.ts ===
export interface Scalar {
(): string;
value: number;
>value : number
}

export function scalar(value: string): Scalar {
>scalar : (value: string) => Scalar
>value : string

return null as any;
>null as any : any
>null : null
}
=== tests/cases/compiler/src/settings/spacing.ts ===
import { scalar } from '../lib/operators/scalar';
>scalar : (value: string) => import("tests/cases/compiler/src/lib/operators/scalar").Scalar

export default {
>{ get xs() { return scalar("14px"); }} : { readonly xs: import("tests/cases/compiler/src/lib/operators/scalar").Scalar; }

get xs() {
>xs : import("tests/cases/compiler/src/lib/operators/scalar").Scalar

return scalar("14px");
>scalar("14px") : import("tests/cases/compiler/src/lib/operators/scalar").Scalar
>scalar : (value: string) => import("tests/cases/compiler/src/lib/operators/scalar").Scalar
>"14px" : "14px"
}
};

55 changes: 55 additions & 0 deletions tests/baselines/reference/declarationEmitPrefersPathKindBasedOnBundling2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//// [tests/cases/compiler/declarationEmitPrefersPathKindBasedOnBundling2.ts] ////

//// [scalar.ts]
export interface Scalar {
(): string;
value: number;
}

export function scalar(value: string): Scalar {
return null as any;
}
//// [spacing.ts]
import { scalar } from '../lib/operators/scalar';

export default {
get xs() {
return scalar("14px");
}
};


//// [dist.js]
define("lib/operators/scalar", ["require", "exports"], function (require, exports) {
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
function scalar(value) {
return null;
}
exports.scalar = scalar;
});
define("settings/spacing", ["require", "exports", "lib/operators/scalar"], function (require, exports, scalar_1) {
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.default = {
get xs() {
return scalar_1.scalar("14px");
}
};
});


//// [dist.d.ts]
declare module "lib/operators/scalar" {
export interface Scalar {
(): string;
value: number;
}
export function scalar(value: string): Scalar;
}
declare module "settings/spacing" {
const _default: {
readonly xs: import("lib/operators/scalar").Scalar;
};
export default _default;
}
29 changes: 29 additions & 0 deletions tests/baselines/reference/declarationEmitPrefersPathKindBasedOnBundling2.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
=== tests/cases/compiler/src/lib/operators/scalar.ts ===
export interface Scalar {
>Scalar : Symbol(Scalar, Decl(scalar.ts, 0, 0))

(): string;
value: number;
>value : Symbol(Scalar.value, Decl(scalar.ts, 1, 12))
}

export function scalar(value: string): Scalar {
>scalar : Symbol(scalar, Decl(scalar.ts, 3, 1))
>value : Symbol(value, Decl(scalar.ts, 5, 23))
>Scalar : Symbol(Scalar, Decl(scalar.ts, 0, 0))

return null as any;
}
=== tests/cases/compiler/src/settings/spacing.ts ===
import { scalar } from '../lib/operators/scalar';
>scalar : Symbol(scalar, Decl(spacing.ts, 0, 8))

export default {
get xs() {
>xs : Symbol(xs, Decl(spacing.ts, 2, 16))

return scalar("14px");
>scalar : Symbol(scalar, Decl(spacing.ts, 0, 8))
}
};

32 changes: 32 additions & 0 deletions tests/baselines/reference/declarationEmitPrefersPathKindBasedOnBundling2.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
=== tests/cases/compiler/src/lib/operators/scalar.ts ===
export interface Scalar {
(): string;
value: number;
>value : number
}

export function scalar(value: string): Scalar {
>scalar : (value: string) => Scalar
>value : string

return null as any;
>null as any : any
>null : null
}
=== tests/cases/compiler/src/settings/spacing.ts ===
import { scalar } from '../lib/operators/scalar';
>scalar : (value: string) => import("tests/cases/compiler/src/lib/operators/scalar").Scalar

export default {
>{ get xs() { return scalar("14px"); }} : { readonly xs: import("tests/cases/compiler/src/lib/operators/scalar").Scalar; }

get xs() {
>xs : import("tests/cases/compiler/src/lib/operators/scalar").Scalar

return scalar("14px");
>scalar("14px") : import("tests/cases/compiler/src/lib/operators/scalar").Scalar
>scalar : (value: string) => import("tests/cases/compiler/src/lib/operators/scalar").Scalar
>"14px" : "14px"
}
};

22 changes: 22 additions & 0 deletions tests/cases/compiler/declarationEmitPrefersPathKindBasedOnBundling.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// @declaration: true
// @target: es5
// @baseUrl: /.src/tests/cases/compiler
// @outDir: ./dist
// @rootDir: ./tests/cases/compiler/src
// @filename: src/lib/operators/scalar.ts
export interface Scalar {
(): string;
value: number;
}

export function scalar(value: string): Scalar {
return null as any;
}
// @filename: src/settings/spacing.ts
import { scalar } from '../lib/operators/scalar';

export default {
get xs() {
return scalar("14px");
}
};
23 changes: 23 additions & 0 deletions tests/cases/compiler/declarationEmitPrefersPathKindBasedOnBundling2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// @declaration: true
// @target: es5
// @baseUrl: /.src/tests/cases/compiler
// @module: amd
// @outFile: ./dist.js
// @rootDir: ./tests/cases/compiler/src
// @filename: src/lib/operators/scalar.ts
export interface Scalar {
(): string;
value: number;
}

export function scalar(value: string): Scalar {
return null as any;
}
// @filename: src/settings/spacing.ts
import { scalar } from '../lib/operators/scalar';

export default {
get xs() {
return scalar("14px");
}
};