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

Adds 'lib' reference directives #23893

Merged
merged 16 commits into from
Jun 4, 2018
Merged

Adds 'lib' reference directives #23893

merged 16 commits into from
Jun 4, 2018

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented May 4, 2018

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.

NOTE: Apparently we were very close to running out of heap memory when running gulp tests after running gulp clean as gulp is holding onto too much memory. Individually each task is fine, but in concert they add up to a large amount of uncollected memory. To address this, I've added a script to run some of our builds out-of-process. This is not an issue in the jake builds.

Fixes #15732.
Supersedes #15780

forEach(toArray(entryOrList), (arg: PragmaPsuedoMap["reference"]) => {
if (arg.arguments["no-default-lib"]) {
context.hasNoDefaultLib = true;
}
else if (arg.arguments.types) {
typeReferenceDirectives.push({ pos: arg.arguments.types.pos, end: arg.arguments.types.end, fileName: arg.arguments.types.value });
}
else if (arg.arguments.lib) {
Copy link

Choose a reason for hiding this comment

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

#23904 Could avoid repeated code here

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you want me to do this or do you want to do in your other PR once this is merged?

Copy link

Choose a reason for hiding this comment

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

I'll close the other PR since it doesn't have much benefit with only 2 branches, but maybe it would have a benefit with 3 branches, or there's a better way to share the code.

@ghost ghost mentioned this pull request May 4, 2018
>Array : ArrayConstructor
>from : { <T>(iterable: Iterable<T> | ArrayLike<T>): T[]; <T, U>(iterable: Iterable<T> | ArrayLike<T>, mapfn: (v: T, k: number) => U, thisArg?: any): U[]; <T>(arrayLike: ArrayLike<T>): T[]; <T, U>(arrayLike: ArrayLike<T>, mapfn: (v: T, k: number) => U, thisArg?: any): U[]; }
>from : { <T>(arrayLike: ArrayLike<T>): T[]; <T, U>(arrayLike: ArrayLike<T>, mapfn: (v: T, k: number) => U, thisArg?: any): U[]; <T>(iterable: Iterable<T> | ArrayLike<T>): T[]; <T, U>(iterable: Iterable<T> | ArrayLike<T>, mapfn: (v: T, k: number) => U, thisArg?: any): U[]; }
Copy link

Choose a reason for hiding this comment

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

Shouldn't arrayFrom.ts have a Set test? Then if would break due to this change since Set is Iterable but not ArrayLike.

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'll add a test, but this shouldn't break because there is still an overload that supports Iterable. The order just changed because I cleaned up the default order of libs. Previously if you had a "lib": ["es5", "es2015"] in your tsconfig.json we would actually end up inserting them in reverse order because we were doing files.unshift for default libs.

@NN---
Copy link

NN--- commented May 20, 2018

Will this allow fixing #16077 ?

@@ -1749,7 +1749,7 @@ namespace ts {
* Case-sensitive comparisons compare both strings one code-point at a time using the integer
* value of each code-point after applying `toUpperCase` to each string. We always map both
* strings to their upper-case form as some unicode characters do not properly round-trip to
* lowercase (such as `` (German sharp capital s)).
* lowercase (such as `ẞ` (German sharp capital s)).
Copy link

Choose a reason for hiding this comment

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

Was this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what caused that to change, but I'll revert.

// Internally we add some additional lib references that we only support when used as part of a
// "lib" reference directive. They are not available on the command line or in tsconfig.json.
/* @internal */
export const libMap = cloneMap(commandLineLibMap)
Copy link
Contributor

Choose a reason for hiding this comment

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

i would just add this to the map. chances are no one will ever include it.. and if they did.. it does not break anything.

@@ -0,0 +1,14 @@
/// <reference lib="es5" />
Copy link
Contributor

@mhegazy mhegazy May 30, 2018

Choose a reason for hiding this comment

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

should not this just be:

/// <reference lib="es2015" />
/// <reference lib="dom" />
/// <reference lib="dom.iterable" />
/// <reference lib="webworker.importscripts" />
/// <reference lib="scripthost" />

/// <reference lib="dom" />
/// <reference lib="webworker.importscripts" />
/// <reference lib="scripthost" />
/// <reference lib="dom.iterable" />
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. i would move this just after dom

@@ -1,3 +1,5 @@
/// <reference lib="webworker.importscripts" />
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 needed here, the file is autogenerated, and it already has the correct definition. we add it to lib.d.ts to partially allow the lib to be used in webworkers for simple cases..

the two files dom and webworker have too many conflicts in between them, and can not be loaded together in the same context anyways..

so I would just revert the changes to this file, and leave it as is.

@@ -46,57 +47,5 @@
"webworker.generated": "lib.webworker.d.ts",
"es5.full": "lib.d.ts",
"es2015.full": "lib.es6.d.ts"
},
"sources": {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove the support for sources from jake and gulp as well.

@@ -1,6 +1,55 @@
namespace ts {
/* @internal */
export const compileOnSaveCommandLineOption: CommandLineOption = { name: "compileOnSave", type: "boolean" };

// NOTE: The order here is important to default lib ordering
Copy link
Contributor

Choose a reason for hiding this comment

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

i would expand on this comment and say that the order of the list here is the same order the lib files have in the generated program, and add a note to see use in createProgram

@@ -89,6 +89,7 @@ namespace ts.GoToDefinition {
}

export function getReferenceAtPosition(sourceFile: SourceFile, position: number, program: Program): { fileName: string, file: SourceFile } | undefined {
debugger;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

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

one debugger statement that needs to be removed

@rbuckton rbuckton merged commit fbeb58a into master Jun 4, 2018
@styfle
Copy link
Contributor

styfle commented Jun 4, 2018

Great job with this PR!

The diff won't load for me so I'll just ask here 🤔 ...

Does this PR actually implement <reference lib> in such a way to make the TS engine smaller? Or is this PR just implementing the feature itself and the usage will be implemented in a different PR?

For more context see: #23339 (comment)

Thanks! 👍

@mhegazy mhegazy deleted the libReference branch June 5, 2018 00:03
@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2018

lib folder should go down by 4 MB or so.

@RyanCavanaugh
Copy link
Member

@mhegazy @rbuckton This is going to be a big break to API consumers loading just lib.d.ts from the TS and expecting things to work - what's the mitigation here for APIs loading e.g. a fixed set of files from the network on load?

@weswigham
Copy link
Member

You could always just use an older copy of the lib, if you're one of those API consumers. We ship the latest one with the latest features for consumers, but if you have constraints that force you to, usually you can just include an older copy of the lib in your project - a bunch of our older RWC tests do that.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 6, 2018

If we get feedback on that we can ship an all in one file as well. The biggest issue is tool authors really, like TS-loader for instance. Most users use the built in lib with no changes.

@Bnaya
Copy link

Bnaya commented Jun 9, 2018

Will it add the library to the entire program or only from the reference point and downward?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 11, 2018

not sure i understand the question. the only difference is instead of on file lib.d.ts you will have a set of them in your program.

@RyanCavanaugh
Copy link
Member

@rbuckton @mhegazy this has broken our own build because buildProtocol builds up its program with --noResolve. Checking out fbeb58a and running jake LKG twice fails.

Now noResolve effectively means you don't load any standard library types at all, which will be visible to more than just API consumers.

@ericdrobinson ericdrobinson mentioned this pull request Jun 13, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jun 13, 2018

Looks like we need to ignore noResolve when we are resolving lib files as well.. we already have a ignoreNoDefaultLib flag, so we can extend this to mean resolvingLibReference and ignore noResolve as well as noDefaultLib

@RyanCavanaugh
Copy link
Member

--disableIgnoreSkipNoResolve

@rbuckton
Copy link
Member Author

I'll take a look. Lib refs are supposed to ignore noResolve.

@rbuckton
Copy link
Member Author

This had nothing to do with noResolve and more to do with buildProtocol.ts looking only ignoring "lib.d.ts". It also looks like its already fixed in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Add "lib" reference directive
7 participants