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

Add target: "es2022" #46291

Merged
merged 10 commits into from
Nov 4, 2021
Merged

Add target: "es2022" #46291

merged 10 commits into from
Nov 4, 2021

Conversation

saschanaz
Copy link
Contributor

@saschanaz saschanaz commented Oct 10, 2021

Fixes #44571, fixes #45512

@typescript-bot
Copy link
Collaborator

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

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Oct 10, 2021
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Oct 11, 2021

While it's not ideal that it's not in the beta, this probably should go in 4.5.

@amcasey
Copy link
Member

amcasey commented Oct 11, 2021

Protocol change looks fine 👍

src/lib/es2022.array.d.ts Outdated Show resolved Hide resolved
* 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;
Copy link
Member

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.

Copy link

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

Copy link
Contributor Author

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.

Copy link

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

Copy link
Member

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.

Copy link

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

Copy link
Contributor Author

@saschanaz saschanaz Oct 28, 2021

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.

Copy link
Contributor

@orta orta Oct 28, 2021

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

@andrewbranch andrewbranch merged commit ae582a2 into microsoft:main Nov 4, 2021
@saschanaz saschanaz deleted the es2022 branch November 4, 2021 18:23
@sandersn sandersn mentioned this pull request Nov 5, 2021
@@ -6750,6 +6752,7 @@ namespace ts {
AssertTypeScript = ContainsTypeScript,
AssertJsx = ContainsJsx,
AssertESNext = ContainsESNext,
AssertES2022 = ContainsES2022,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@tychenjiajun
Copy link

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?

@PabloLION
Copy link

Hi team. In playground v4.5.2, Array.prototype.at is still not supported. How should I configure TS to suppress the error?

@saschanaz
Copy link
Contributor Author

This didn't enter 4.5, so you should wait for 4.6 or use nightly.

@rbuckton
Copy link
Member

ES2022 contains private fields and static {}.

@saschanaz
Copy link
Contributor Author

Yup, that's #47018

@PabloLION
Copy link

Thanks for the clarification

mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
* 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 <[email protected]>
This was referenced Feb 1, 2022
@bennypowers
Copy link

When useDefineForClassFields is false, es2022 transforms simple static class fields into class static init blocks. This is a problem for users that need useDefineForClassFields (lit requires this). As well, class fields and class private members are widely supported, where class static init blocks are not. Class fields have smaller bytecounts than transformed output, and are easier to read. I understand that the code is equivalent on an engine which supports static class blocks, but this presents a serious problem for some users, and I think TSC should provide an escape hatch.

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 es2022 to get native private class members, which are widely supported, but they'll get stuck with static init blocks that they didn't write in source - a syntax error in most toolchains and on apple's monopoly engine webkit

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.

@saschanaz
Copy link
Contributor Author

That looks very much like a bug, please file it as a separate issue. Thanks

@bennypowers
Copy link

That looks very much like a bug, please file it as a separate issue. Thanks

#48145

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Support Array.prototype.at Add target: es2022