-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Add target: "es2022"
#46291
Add target: "es2022"
#46291
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
While it's not ideal that it's not in the beta, this probably should go in 4.5. |
Protocol change looks fine 👍 |
src/lib/es2022.array.d.ts
Outdated
* Returns the item located at the specified index. | ||
* @param index The zero-based index of the desired code unit. A negative index will count back from the last item. | ||
*/ | ||
at(index?: number): T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want to make this optional? slice()
makes sense because it's common to copy an entire array, I don't know if allowing forgotten arguments is worth supporting the idiom of peeking at the front.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If leaving it with no parameter defaults to 0 and is valid, why not? Typescript is supposed to pick up on runtime errors, which this is not one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be possible with optional parameter:
// some function that omitting parameter actually makes sense
function foo(index?: number) {
// ...
const item = bar.indexAt(index);
// ...
}
But I'm not sure how many people would do that. I'm happy to revert the last commit but I wonder what others think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any function would work like that for that matter. I think if it’s supported it should be kept like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typescript is supposed to pick up on runtime errors, which this is not one
We have lots of precedent of not supporting everything under the sun just because it doesn't crash (e.g. #15122). If it's plausibly an error, we occasionally err on being restrictive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely not. This situation is much different from the one above because that one doesn't make sense and has alternatives. This here has an alternative, which is passing an explicit 0, but when running .at() it would make sense for it to default to the first element in the array. This doesn't seem like another one of JS's quirks, but in fact very much intentional and should stay like this, unless a maintainer says otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find any discussion from TC39 that says this is intentional, though. Happy to see one if exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to complicate this merge late in the game, but I think we want at(index: number): T | undefined;
- #45512 (comment)
WRT the idea of index?: number
vs index: number
I don't personally have a strong opinion either way, but if forced I think we should drop the ?
and go with index: number
because I think it matches the intention of at(0)
which feels different to at(undefined)
/at()
because the omission doesn't really communicate the same thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two MS engineers prefer not to making it optional 👍
I'll add | undefined
and the newly accepted Error Cause proposal tomorrow (unless anyone prefers to making it a separate PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find any discussion from TC39 that says this is intentional, though. Happy to see one if exists.
I don't think there was explicit discussion, but certainly as a member of TC39 it is my view that at
coerces undefined
to 0
only because everything similar in the language works that way, not because we actually think people should pass undefined
or omit the parameter. By analogy, you can also call it with '0'
, but that doesn't mean | '0'
belongs in the signature.
As a user, if I ever saw at
being called with undefined
or without an argument, I'd assume that was a bug.
This reverts commit e67d6e5.
@@ -6750,6 +6752,7 @@ namespace ts { | |||
AssertTypeScript = ContainsTypeScript, | |||
AssertJsx = ContainsJsx, | |||
AssertESNext = ContainsESNext, | |||
AssertES2022 = ContainsES2022, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ES2022 does not contain new syntax, should we skip ContainsES2022
flag (or set ContainsES2022
= ContainsES2021
) to save the bitflag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are (notably class private fields), but just not here yet AFAIK.
const selections = [
{ foo: 1 }, { bar: 2 }
] as const;
type A = typeof selections[0];
type B = typeof selections.at(-2); Will A and B be the same type? |
Hi team. In playground v4.5.2, |
This didn't enter 4.5, so you should wait for 4.6 or use nightly. |
ES2022 contains private fields and |
Yup, that's #47018 |
Thanks for the clarification |
* Add `target: "es2022"` * Add Object.hasOwn * formatToParts is es2018 * ref update * optional parameter * Revert "optional parameter" This reverts commit e67d6e5. * undefined * error cause * Lint fix Co-authored-by: Orta <git@orta.io>
When Don't get me wrong, I think static blocks are super cool, and I'm looking forward to using them, but many users will want to use https://www.typescriptlang.org/play?target=9#code/MYGwhgzhAECC0G8BQ1XQgFzBglsaY0AvNAORikDcSAvkA input: class A {
static a = 'a';
} output: "use strict";
class A {
static { this.a = 'a'; }
} In my case, I have library sources that use decorators and need to be compiled with set semantics, but I also want them to have untranspiled private fields, which are widely supported. Thank you for considering this case. |
That looks very much like a bug, please file it as a separate issue. Thanks |
|
Fixes #44571, fixes #45512