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

Add support for 'lib' reference directive #15780

Closed
wants to merge 17 commits into from
Closed

Conversation

rbuckton
Copy link
Member

This change adds support for a /// <reference lib="name" /> directive, allowing a file to explicitly include an existing built-in lib file.

  • When a built-in lib is referenced via the lib reference directive, any no-default-lib reference directive in the lib file is ignored. This is so that a lib reference in an imported package won't cause your current project to lose its default lib if you haven't specified one.
  • Built-in lib files are referenced in the same fashion as the "lib" compiler option in tsconfig.json (e.g. use lib="es2015" and not lib="lib.es2015.d.ts", etc.).

In the long term this can help polyfill/shim packages like core-js or es6-shim.

Fixes #15732.

@rbuckton rbuckton requested review from mhegazy and a user May 11, 2017 20:37
@@ -7,6 +7,39 @@
namespace ts {
/* @internal */
export const compileOnSaveCommandLineOption: CommandLineOption = { name: "compileOnSave", type: "boolean" };

/* @internal */
export const libMap = createMapFromTemplate({
Copy link

@ghost ghost May 11, 2017

Choose a reason for hiding this comment

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

This just maps x to lib.x.d.ts. Could be a function instead, combined with a set of allowed libs.

Copy link
Member Author

Choose a reason for hiding this comment

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

This must be a Map to be used in the compiler option I pulled it from, below. If I added a function then we would have yet another place to maintain this list (in addition to the jakefile/gulpfile).

Copy link

Choose a reason for hiding this comment

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

It would still be nice to generate this instead of writing it explicitly. Start with const libs = ["es5", ..., "esnext.asynciterable"], generate the map with lib.${x}.d.ts, and you'll no longer need to use arrayFrom(libMap.keys()).

Copy link
Member Author

Choose a reason for hiding this comment

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

Except for the outliers like "es6" and "es7" which are aliases.

Copy link
Member Author

@rbuckton rbuckton May 11, 2017

Choose a reason for hiding this comment

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

Alternatively, we leave libMap as is, and add: export const libs = arrayFrom(libMap.keys());

@@ -9,6 +9,7 @@ namespace ts {
diagnosticMessage?: DiagnosticMessage;
isNoDefaultLib?: boolean;
Copy link

Choose a reason for hiding this comment

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

These three should really be kind?: "no-default-lib" | "types" | "lib".

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or kind: "no-default-lib" | "path" | "types" | "lib".

function processLibReferenceDirectives(file: SourceFile) {
const libDirectory = getLibDirectory();
forEach(file.libReferenceDirectives, libReference => {
const libName = libReference.fileName.toLocaleLowerCase();
Copy link

Choose a reason for hiding this comment

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

Do we really need to lower-case it? If they try accessing /// <reference lib="es2017.Object" /> we will correct their spelling to "es2017.object".

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what we do for --lib, so its consistent.

export function getFileReferenceFromReferencePath(comment: string, commentRange: CommentRange): ReferencePathMatchResult {
const simpleReferenceRegEx = /^\/\/\/\s*<reference\s+/gim;
const isNoDefaultLibRegEx = /^(\/\/\/\s*<reference\s+no-default-lib\s*=\s*)('|")(.+?)\2\s*\/>/gim;
if (simpleReferenceRegEx.test(comment)) {
Copy link

Choose a reason for hiding this comment

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

Good place for an early return.

@ghost
Copy link

ghost commented May 11, 2017

We'll also want to support services (goto definition at least, find-all-references isn't as important) on these, but that should wait for after both this and #15737 are in.

@rbuckton rbuckton requested a review from sandersn May 12, 2017 16:59
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

The ignoreNoDefaultLib parameter is never provided the value true. Is it needed?

*/
export function getSpellingSuggestion<T>(name: string, choices: T[], exclusions: string[] | undefined, getName: (candidate: T) => string | undefined): T | undefined;
export function getSpellingSuggestion<T>(name: string, choices: T[], exclusions?: string[], getName: (candidate: T) => string | undefined = identity): T | undefined {
// If there is a candidate that's the same except for case, return that.
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this back to the JSDoc?

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 didn't want to repeat the rather lengthy description for both overloads, and its a lot of text to show when you mouse over a reference. Its too bad we don't support the @summary jsdoc tag.

Copy link
Member

Choose a reason for hiding this comment

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

Ouch. I didn't realise that both overloads had separate jsdoc. And @summary is exactly what I was thinking of. Isn't there the opposite, something like @remarks ? (Later: nope, I guess I am thinking of XML doc comments in C#)

}
else {
const libDirectory = host.getDefaultLibLocation ? host.getDefaultLibLocation() : getDirectoryPath(host.getDefaultLibFileName(options));
const libDirectory = getLibDirectory();
Copy link
Member

Choose a reason for hiding this comment

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

this shadows the outer libDirectory. Can you change it so that the code is less confusing?

const libFileName = libMap.get(libName);
if (libFileName) {
// we ignore any 'no-default-lib' reference set on this import.
processRootFile(combinePaths(libDirectory, libFileName), /*isDefaultLib*/ true, /*ignoreNoDefaultLib*/ false);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't be ignoreNoDefaultLib be true in order to ignore 'no-default-lib' references?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Shouldn't flipping false to true for ignoreNoDefaultLib in the last commit change some tests?

@rbuckton
Copy link
Member Author

@sandersn There wasn't a good test to capture that case. I've added one as of the last commit (47ed6cc).

@rbuckton rbuckton mentioned this pull request Jun 6, 2017
@rbuckton
Copy link
Member Author

rbuckton commented Jun 6, 2017

@ahejlsberg: per your comments in the design meeting, this should now be loading each file only once. This means that lib.d.ts and lib.es6.d.ts are no longer concatenated, but instead use lib references to the files that compose them.

See src/lib/default.es5.d.ts for an example of the file that is now used to build lib.d.ts.

@alexeagle
Copy link
Contributor

This came up today in Google style discussion, are you still planning to land this?

@alexeagle
Copy link
Contributor

@DanielRosenwasser we discussed this one next week, would still be nice to be able to use this for our BUILD.bazel file generator (in cases where we require libs to be declared dependencies)

@mhegazy mhegazy mentioned this pull request Nov 1, 2017
@schickling
Copy link

Any progress on this?

@rbuckton
Copy link
Member Author

rbuckton commented May 4, 2018

Superseded by #23893

@rbuckton rbuckton closed this May 4, 2018
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
@RyanCavanaugh RyanCavanaugh deleted the libReferenceDirective branch June 16, 2022 22:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants