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: add es2016.intl.d.ts #39662

Closed
wants to merge 8 commits into from
Closed

fix: add es2016.intl.d.ts #39662

wants to merge 8 commits into from

Conversation

longlho
Copy link
Contributor

@longlho longlho commented Jul 19, 2020

part of #29129

@longlho longlho force-pushed the canonical branch 2 times, most recently from ac865de to affaf0e Compare July 19, 2020 21:33
@typescript-bot
Copy link
Collaborator

It looks like you've sent a pull request to update our 'lib' files. These files aren't meant to be edited by hand, as they consist of last-known good states of the compiler and are generated from 'src'. Unless this is necessary, consider closing the pull request and sending a separate PR to update 'src'.

@typescript-bot typescript-bot added the lib update PR modifies files in the `lib` folder label Jul 19, 2020
@longlho
Copy link
Contributor Author

longlho commented Jul 20, 2020

cc @orta

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

Overall looks good, I think UnicodeBCP47LocaleIdentifier should probably live here and get removed from the 2020 version.

I'll also leave this a few days to let others give some feedback

lib/lib.es2016.intl.d.ts Outdated Show resolved Hide resolved
@orta orta self-assigned this Jul 21, 2020
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Jul 21, 2020
@longlho
Copy link
Contributor Author

longlho commented Aug 14, 2020

ping @orta :)

@longlho
Copy link
Contributor Author

longlho commented Aug 30, 2020

ping @DanielRosenwasser

@DanielRosenwasser
Copy link
Member

Overall looks good, I think UnicodeBCP47LocaleIdentifier should probably live here and get removed from the 2020 version.

Hmm, is that a breaking change?

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Did these lib files need to be added? I think you can get away with only modifying src, and now I'm vaguely suspicious from these having spacing differences.

lib/lib.es2016.intl.d.ts Outdated Show resolved Hide resolved
lib/lib.es2016.intl.d.ts Outdated Show resolved Hide resolved
src/lib/es2016.intl.d.ts Outdated Show resolved Hide resolved
@longlho
Copy link
Contributor Author

longlho commented Aug 30, 2020

That's what I thought but I'm cargo culting and tests seem upset if I just modified src

longlho and others added 3 commits August 31, 2020 22:48
Co-authored-by: Daniel Rosenwasser <[email protected]>
Co-authored-by: Daniel Rosenwasser <[email protected]>
Co-authored-by: Daniel Rosenwasser <[email protected]>
@sandersn
Copy link
Member

sandersn commented Sep 3, 2020

@DanielRosenwasser what do you think? Should we go ahead and merge this? Seems like the only outstanding question is whether to check in the lib/ changes.

@longlho
Copy link
Contributor Author

longlho commented Sep 12, 2020

can I get a verdict here 😄 ? cc @DanielRosenwasser @sandersn

@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@orta
Copy link
Contributor

orta commented Jun 1, 2021

OK, I've rebased this and set it up locally - as it is a project targeting es2020 would get duplicate identifiers for UnicodeBCP47LocaleIdentifier:

❯ node ../../typescript-compiler/built/local/tsc.js
../../typescript-compiler/built/local/lib.es2016.intl.d.ts:29:10 - error TS2300: Duplicate identifier 'UnicodeBCP47LocaleIdentifier'.

29     type UnicodeBCP47LocaleIdentifier = string;
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  ../../typescript-compiler/built/local/lib.es2020.intl.d.ts:29:10
    29     type UnicodeBCP47LocaleIdentifier = string;
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    'UnicodeBCP47LocaleIdentifier' was also declared here.

../../typescript-compiler/built/local/lib.es2020.intl.d.ts:29:10 - error TS2300: Duplicate identifier 'UnicodeBCP47LocaleIdentifier'.

29     type UnicodeBCP47LocaleIdentifier = string;
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  ../../typescript-compiler/built/local/lib.es2016.intl.d.ts:29:10
    29     type UnicodeBCP47LocaleIdentifier = string;
                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    'UnicodeBCP47LocaleIdentifier' was also declared here.


Found 2 errors.

I think we'll need to remove the identifier from es2020 to have it be usable in es2016. The question of whether this is a breaking change is a good one. If you had a setup like:

"target": "es2019",
"lib": ["es2020.intl"]

and were referring to UnicodeBCP47LocaleIdentifier you would still have access to it because of the target. However, if your target was es2015, and you explicitly just referenced the es2020.intl in lib:

"target": "es2015",
"lib": ["es2020.intl"]

Then moving the type back is a breaking change. Which given the breadth of the TS userbase, someone probably has this setup.

Which gives three options:

  • Accept the potential break in the types for target es2015 + a direct reference in lib, and move the identifier to es2016
  • NOOP on this change
  • Switch the type in es2016 to be string instead of the named alias, keeping UnicodeBCP47LocaleIdentifier in es2020

Personally, I fall into the last bucket.

@longlho
Copy link
Contributor Author

longlho commented Jun 1, 2021

I personally would pick the 1st option. ECMA402 doesn't often introduce a lot of new APIs per release and have them work independently without previous version, e.g you should not be able specify es2020.intl without es2019.intl and expect it to properly work. Future versions add new options to existing set of limited APIs and normative changes are made to previous types.
UnicodeBCP47LocaleIdentifier is an example of that, it used to just be BCP47 which includes grandfathered locales but was removed in later versions (see tc39/proposal-intl-locale#63 for discussion).
I think the real change (outside of this PR) is probably to make es2020.intl includes all es2015.intl through out es2019.intl.

@orta
Copy link
Contributor

orta commented Aug 30, 2021

OK, I think this is good to go now:

  • I've removed all the lib references, those changes will get propagated when we update the lkg
  • I've removed the es2020.intl reference to UnicodeBCP47LocaleIdentifier. It's now available in es2016.

This last one needs to get mentioned in the 4.5 release notes breaking changes section. Something like:

If you relied on UnicodeBCP47LocaleIdentifier from es2020.intl it is now available in es2016.intl.

I agree it's pretty niche, but a single sentence should be enough to handle covering all cases during migration.

@orta
Copy link
Contributor

orta commented Aug 30, 2021

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 30, 2021

Heya @orta, I've started to run the parallelized community code test suite on this PR at b840e5c. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@orta
Copy link
Contributor

orta commented Sep 8, 2021

This PR was deprecated and merged into main via #45647 - thanks @longlho !

@orta orta closed this Sep 8, 2021
@ls-andrew-borstein
Copy link

This PR was deprecated and merged into main via #45647

I don't see this code anywhere in #45647 or anywhere else in TS (as mentioned elsewhere). Is that intentional?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug lib update PR modifies files in the `lib` folder
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants