Skip to content

Add es2018.intl ref to es2020.intl #49152

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
merged 2 commits into from
May 20, 2022
Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented May 17, 2022

es2020.intl refers to NumberFormatPartTypes declared in es2018.intl as of #46508.

I'm not sure how to test this; it repros on Definitely Typed in types/ndarray, but when I copy the same files into a compiler test it passes without a problem.

Fixes 3 test failures on DT's overnight run.

Edit: The most recent commit adds a tests to show that the lib change does what it should. But it doesn't show that the bug is fixed -- just that Intl.NumberFormatPartTypes is resolved in a file with /// <reference lib="esnext.bigint" />.

es2020.intl refers to NumberFormatPartTypes declared in es2018.intl
as of #46508.

I'm not sure how to test this; it repros on Definitely Typed in
types/ndarray, but when I copy the same files into a compiler test it
passes without a problem.
@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 17, 2022
@sandersn sandersn requested a review from gabritto May 17, 2022 22:33
@sandersn
Copy link
Member Author

sandersn commented May 17, 2022

For comparison, here is the test I tried:

// @Filename: tsconfig.json
{
    "compilerOptions": {
        "lib": [
            "es6"
        ],
        "types": [],
        "noEmit": true
    }
}

// @Filename: ndarray-test.ts
/// <reference lib="esnext.bigint" />

and here is the failing DT package after cutting out unnecessary code:

// @Filename: ndarray-test.ts
/// <reference lib="esnext.bigint" />
// @Filename: tsconfig.json
{
    "compilerOptions": {
        "lib": [
            "es6"
        ],
        "types": [],
        "noEmit": true,
    },
    "files": [
        "ndarray-tests.ts"
    ]
}

Those two files also fail on their own, outside the DT repo.
Triple-slash references work elsewhere in the compiler tests.

@gabritto
Copy link
Member

I think in general I don't understand how lib files supposed to work together. In this case, both the es2020.intl and es2018.intl have declare namespace Intl, so those will be merged if the two lib files are present, right? And before this PR and the PR that introduced the type dependency, it was possible to have just the es2020.intl file, no? So won't adding this dependency (1) change the type of things due to merging, or (2) include es2018 declarations that should not be there?

It doesn't actually show that the original bug has been fixed,
though.
@sandersn
Copy link
Member Author

sandersn commented May 20, 2022

  1. Yes, declare namespace Intl will merge across files.
  2. Before this PR, it was possible to have just es2020.intl, and pre-fix: update types for RTF.p.formatToParts() result #46508, the types were actually independent.
  3. This PR could change the types, but I'm pretty sure nothing conflicts between es2020 and es2018.
  4. In almost all cases, es2020.intl implies the existence of es2018.intl. I guess somebody could make a custom runtime where that's not true. Several other lib files depend on those of previous years, so there's precedent for this.

@sandersn
Copy link
Member Author

I'm going to merge this because I'm confident of the fix, the test at least shows that it does a related, correct thing, and I'm out of time to debug it.

@sandersn sandersn merged commit 006ae33 into main May 20, 2022
@sandersn sandersn deleted the add-es2018.intl-ref-to-es2020.intl branch May 20, 2022 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants