Uh oh!
There was an error while loading. Please reload this page.
Update bigint declaration file - #38526
Conversation
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
Also needs to make NumberFormat.format support bigint in order to fix #36970.
Uh oh!
There was an error while loading. Please reload this page.
| /** | ||
| * The locale matching algorithm to use.The default is "best fit". For information about this option, see the {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl#Locale_negotiation Intl page}. | ||
| */ | ||
| localeMatcher?: "lookup" | "best fit" | string; |
There was a problem hiding this comment.
"x" | string is currently reduced to string and the literals are thrown out. I think string is fine for now.
There was a problem hiding this comment.
The feature request for supporting this is #33471
There was a problem hiding this comment.
Do I need to remove these "x", or just leave them here, waiting for the benefit of this feature request?
There was a problem hiding this comment.
It's a fairly complex feature and not likely to be in 4.0 or 4.1. We should stick with string for all these unions.
| } | ||
| interface BigIntToLocaleStringOptionsStyleUnit extends Omit<BigIntToLocaleStringOptionsBase, "style"> { | ||
| style: "unit"; |
There was a problem hiding this comment.
All you get for the overhead of these two interfaces+union is the ability to make unit appear only when style: "unit". Considering that every other field in BigIntToLocaleStringOptionsBase is optional, I think it's better to put both unit and currency in the base as optional too, then get rid of the subtypes.
There was a problem hiding this comment.
Yeah.
In fact, "unit" and "currency" are already in base class as optional.
I would like to use "unit" as example to explain why I did this:
If "style" is not "unit", the value of "unit" is arbitrary, but if "style" is "unit" and "unit" is undefined, there would be an error. With this, user could know the error while type checking.
So which side does the responsibility to avoid error belongs to? The user-side or ts-definition?
I prefer latter one, but both are reasonable.
Anyway, I would follow your final decision.
There was a problem hiding this comment.
The tradeoff is that the standard library is more complicated and slower to check. I'd like to go with the simpler single-interface solution and let users avoid the error.
Uh oh!
There was an error while loading. Please reload this page.
Song Gao (ShuiRuTian)
commented
Jun 18, 2020
Friendly ping Nathan Shively-Sanders (@sandersn) ~ |
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
Also needs to make NumberFormat.format support bigint in order to fix #36970.
| /** | ||
| * The locale matching algorithm to use.The default is "best fit". For information about this option, see the {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl#Locale_negotiation Intl page}. | ||
| */ | ||
| localeMatcher?: "lookup" | "best fit" | string; |
There was a problem hiding this comment.
It's a fairly complex feature and not likely to be in 4.0 or 4.1. We should stick with string for all these unions.
| } | ||
| interface BigIntToLocaleStringOptionsStyleUnit extends Omit<BigIntToLocaleStringOptionsBase, "style"> { | ||
| style: "unit"; |
There was a problem hiding this comment.
The tradeoff is that the standard library is more complicated and slower to check. I'd like to go with the simpler single-interface solution and let users avoid the error.
Thanks for the PR, Song Gao (@ShuiRuTian)! |
* upstream/master: (58 commits) Variadic tuple types (microsoft#39094) chore: resolve suggestions Expand auto-import to all package.json dependencies (microsoft#38923) inline local functions Update bigint declaration file (microsoft#38526) Update user baselines (microsoft#39077) LEGO: check in for master to temporary branch. Add missing index.ts files to user projects (microsoft#39163) Add reason for a disabled code action (microsoft#37871) Minor fix for assertion predicates (microsoft#38710) Update LKG (microsoft#39173) Reparse top level 'await' in modules (microsoft#39084) change chore: more change chore: resolve review chore: save space fix: lint error test: add test for it chore: make isJsxAttr required chore: revert change in checker ... # Conflicts: # src/compiler/binder.ts # src/compiler/checker.ts # src/compiler/parser.ts # src/compiler/types.ts
* update toLocalString function signature * update test. * fix lint * follow review advice. * format and better comment. * format * add case * fix symbol. * remove subtype and string union in interface. * remove useless code. Co-authored-by: Song Gao <song.gao@laserfiche.com>
Fixes#36970