Skip to content

fix: update types for RTF.p.formatToParts() result #46508

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 1 commit into from
May 10, 2022

Conversation

justingrant
Copy link
Contributor

This PR:

Fixes #46245

@orta - what are the right tests to add for this kind of change?

@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 lib update PR modifies files in the `lib` folder For Milestone Bug PRs that fix a bug with a specific milestone labels Oct 25, 2021
@ghost
Copy link

ghost commented Oct 25, 2021

CLA assistant check
All CLA requirements met.

@justingrant
Copy link
Contributor Author

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'.

I was a little confused by these dual files because only the lib versions had links to the relevant parts of the ECMA-402 specification. If we're not allowed to change lib files in a PR, then how to add a specification link for a brand-new type?

@DanielRosenwasser
Copy link
Member

You'll need to make the update in src/lib, sorry that the message isn't clearer.

@justingrant
Copy link
Contributor Author

You'll need to make the update in src/lib, sorry that the message isn't clearer.

@DanielRosenwasser the src/lib files don't contain links to the spec, only to MDN. The files in lib do contain spec links. If I'm not supposed to edit lib, then is there some other file that I need to edit to map APIs to spec links? If not, how do the spec links get there?

@justingrant
Copy link
Contributor Author

You'll need to make the update in src/lib, sorry that the message isn't clearer.

@DanielRosenwasser the src/lib files don't contain links to the spec, only to MDN. The files in lib do contain spec links. If I'm not supposed to edit lib, then is there some other file that I need to edit to map APIs to spec links? If not, how do the spec links get there?

Still hoping to get an answer to this question.

@DanielRosenwasser
Copy link
Member

Sorry, if they're not from src/lib, they're from https://github.com/microsoft/TypeScript-DOM-lib-generator

@DanielRosenwasser
Copy link
Member

I've updated the bot's message for the future, thanks for pointing out that it wasn't clear, and sorry for the confusion!

@justingrant
Copy link
Contributor Author

Sorry, if they're not from src/lib, they're from https://github.com/microsoft/TypeScript-DOM-lib-generator

@DanielRosenwasser - Sorry I still don't understand what are the right next steps in this PR.

  1. Should I revert changes to lib/lib.es2020.intl.d.ts?
  2. If (1) is "yes" then should I put links to the ES spec in my changes to src/lib/es2020.intl.d.ts?
  3. If (2) is "no" then how will the spec links get into lib/lib.es2020.intl.d.ts, given that the most relevant spec link is not what's linked by MDN but is a bit deeper in the 402 spec?

The specific spec link I'm concerned about is https://tc39.es/ecma402/#sec-singularrelativetimeunit which is the most relevant link for the RelativeTimeFormatUnitSingular type that's added in this PR. AFAICT, this spec link is not present in MDN at all, so I'm not sure how it can get into lib/lib.es2020.intl.d.ts without me adding the link somewhere.

@DanielRosenwasser
Copy link
Member

Okay, I reread the changes - a double apology for the misunderstanding.

You'll need to back out lib because we only change those files when we update the last-known-good state of the compiler, or create a release. Your changes in src will eventually make their way into a published release though.

@justingrant
Copy link
Contributor Author

You'll need to back out lib because we only change those files when we update the last-known-good state of the compiler, or create a release. Your changes in src will eventually make their way into a published release though.

@DanielRosenwasser - OK that answers question (1) above, but not the other two questions:

  • If (1) is "yes" then should I put links to the ECMA-402 spec in my changes to src/lib/es2020.intl.d.ts?
  • If (2) is "no" then how will the spec links get into lib/lib.es2020.intl.d.ts, given that the most relevant spec link is not what's linked by MDN but is a bit deeper in the 402 spec?

@DanielRosenwasser
Copy link
Member

If it's important, would it just make more sense to add content to MDN than to deep link to the spec? I don't think most users are interested in parsing spec text.

This commit updates the type of `RelativeTimeFormatPart` to clarify that
the `unit` prop is always singular, unlike the plural or singular values
that are accepted as inputs.

This also changes `RelativeTimeFormatPart` to be a discriminated
union type because the `unit` prop is only present if the `type` prop's
value is not "literal".

Fixes microsoft#46245
@justingrant justingrant force-pushed the relative-time-format-plural branch from ef5fc0c to 150ccd1 Compare November 16, 2021 00:12
@justingrant
Copy link
Contributor Author

If it's important, would it just make more sense to add content to MDN than to deep link to the spec? I don't think most users are interested in parsing spec text.

I assumed it was mostly needed for the TS team to verify that the types match the spec. Anyway, sounds like it's not needed so I just reverted the changes to lib/lib.es2020.intl.d.ts as requested. This PR should now be ready to merge unless there's other concerns. Thanks!

@sandersn sandersn assigned sandersn and unassigned orta Feb 25, 2022
@sandersn sandersn merged commit 8bf45a4 into microsoft:main May 10, 2022
sandersn added a commit that referenced this pull request 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.
sandersn added a commit that referenced this pull request May 20, 2022
* Add es2018.intl ref to es2020.intl

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.

* Add a test that shows the change works

It doesn't actually show that the original bug has been fixed,
though.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone lib update PR modifies files in the `lib` folder
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Inaccurate type for return value of Intl.RelativeTimeFormat.prototype.formatToParts
5 participants