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

Fix import order #37510

Merged
merged 1 commit into from
Mar 26, 2020
Merged

Conversation

falsandtru
Copy link
Contributor

Separated from #36368

@falsandtru
Copy link
Contributor Author

/// <reference lib="es2015.promise" />
/// <reference lib="es2015.iterable" />

@falsandtru
Copy link
Contributor Author

Note that the problem is that the order of overloads wrongly looks like

    race<T>(values: Iterable<T | PromiseLike<T>>): Promise<awaited T>;
    race<T>(values: readonly (T | PromiseLike<T>)[]): Promise<awaited T>;

from the current definition

/// <reference lib="es2015.generator" />
/// <reference lib="es2015.promise" />
/// <reference lib="es2015.iterable" />

but actually

    race<T>(values: readonly (T | PromiseLike<T>)[]): Promise<awaited T>;
    race<T>(values: Iterable<T | PromiseLike<T>>): Promise<awaited T>;

because es2015.generator loads es2015.iterable. This misleading confuses us.

@rbuckton
Copy link
Member

This won't affect the resolution order, as the order is explicitly specified in commandLineParser.ts

@falsandtru
Copy link
Contributor Author

falsandtru commented Mar 25, 2020

This fixes the definition order which doesn't match the resolution order.

@rbuckton
Copy link
Member

The order they are specified in the lib files shouldn't matter in the slightest. The order they are loaded (and the order of the definitions) should come from the libEntries array in commandLineParser.ts. We did this explicitly so that it didn't matter what order you specify them in your "libs" or via /// <reference /> directives so that we would preserve the correct ordering of overloads.

@falsandtru
Copy link
Contributor Author

The current definition order sometimes confuses us as I mentioned at #37510 (comment). And as you said "This won't affect the resolution order", you have no reason to keep the current definition order which doesn't match the resolution order.

@DanielRosenwasser
Copy link
Member

I think we need this for the playground to generate a reasonable lib.d.ts.

@typescript-bot pack this
@typescript-bot cherry-pick this to release-3.9 and LKG

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2020

Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at b9a7fe0. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2020

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-3.9 on this PR at b9a7fe0. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 26, 2020

I agree that this list should at least strive to match the one that we reorder automatically. Not doing so has caused issues (see microsoft/TypeScript-Website#409). Maybe the list should start with a leading comment telling users to keep the list in sync.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Mar 26, 2020
Component commits:
b9a7fe0 Fix import order
@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #37628 for you.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2020

Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/69479/artifacts?artifactName=tgz&fileId=C4560692EC15BFD47A3351E8EA2EFA809439F15818B45E94B0031DEECD2D2C9402&fileName=/typescript-3.9.0-insiders.20200326.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 26, 2020

Yup, it definitely matters. Just annoying that I have to redo the VS-side syncing now even though it won't be affected.

@DanielRosenwasser DanielRosenwasser merged commit 7f1df6e into microsoft:master Mar 26, 2020
@DanielRosenwasser
Copy link
Member

Thanks @falsandtru!

DanielRosenwasser pushed a commit that referenced this pull request Mar 26, 2020
* Cherry-pick PR #37510 into release-3.9

Component commits:
b9a7fe0 Fix import order

* Update LKG

Co-authored-by: falsandtru <falsandtru@users.noreply.github.com>
Co-authored-by: typescript-bot <typescript@microsoft.com>
@falsandtru falsandtru deleted the lib/iterable branch March 26, 2020 21:04
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.

None yet

5 participants