-
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 at method #40697
Add at method #40697
Conversation
Up. |
This proposal has web combability problem so it's need to be renamed. |
FYI, per the linked issue, |
The TypeScript team hasn't accepted the linked issue #40695. If you can get it accepted, this PR will have a better chance of being reviewed. |
@@ -295,7 +295,11 @@ namespace ts { | |||
file: undefined, | |||
start: 0, | |||
length: 0, | |||
messageText: "Argument for '--lib' option must be: 'es5', 'es6', 'es2015', 'es7', 'es2016', 'es2017', 'es2018', 'esnext', 'dom', 'dom.iterable', 'webworker', 'webworker.importscripts', 'scripthost', 'es2015.core', 'es2015.collection', 'es2015.generator', 'es2015.iterable', 'es2015.promise', 'es2015.proxy', 'es2015.reflect', 'es2015.symbol', 'es2015.symbol.wellknown', 'es2016.array.include', 'es2017.object', 'es2017.sharedmemory', 'es2017.string', 'es2017.intl', 'es2017.typedarrays', 'es2018.asynciterable', 'es2018.intl', 'es2018.promise', 'es2018.regexp', 'esnext.array', 'esnext.symbol', 'esnext.intl', 'esnext.bigint', 'esnext.bigint', 'esnext.string', 'esnext.promise'.", | |||
messageText: "Argument for '--lib' option must be: 'es5', 'es6', 'es2015', 'es7', 'es2016', 'es2017', 'es2018', 'esnext', 'dom', 'dom.iterable', 'webworker', " + |
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.
It's painful when you edit something on a very very very long line.
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.
We should convert these into baseline/snapshot tests. Not necessary for you to do in this PR, so I've filed #44950 to keep track of the issue.
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.
Should I revert these changes?...
Co-authored-by: Anonymous <65428781+CrimsonCodes0@users.noreply.github.com>
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.
Assuming there are no further web compatibility issues with .at
, this looks good to me. The feature is shipping in both v8 and SpiderMonkey, so it seems fairly stable.
@@ -295,7 +295,11 @@ namespace ts { | |||
file: undefined, | |||
start: 0, | |||
length: 0, | |||
messageText: "Argument for '--lib' option must be: 'es5', 'es6', 'es2015', 'es7', 'es2016', 'es2017', 'es2018', 'esnext', 'dom', 'dom.iterable', 'webworker', 'webworker.importscripts', 'scripthost', 'es2015.core', 'es2015.collection', 'es2015.generator', 'es2015.iterable', 'es2015.promise', 'es2015.proxy', 'es2015.reflect', 'es2015.symbol', 'es2015.symbol.wellknown', 'es2016.array.include', 'es2017.object', 'es2017.sharedmemory', 'es2017.string', 'es2017.intl', 'es2017.typedarrays', 'es2018.asynciterable', 'es2018.intl', 'es2018.promise', 'es2018.regexp', 'esnext.array', 'esnext.symbol', 'esnext.intl', 'esnext.bigint', 'esnext.bigint', 'esnext.string', 'esnext.promise'.", | |||
messageText: "Argument for '--lib' option must be: 'es5', 'es6', 'es2015', 'es7', 'es2016', 'es2017', 'es2018', 'esnext', 'dom', 'dom.iterable', 'webworker', " + |
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.
We should convert these into baseline/snapshot tests. Not necessary for you to do in this PR, so I've filed #44950 to keep track of the issue.
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.
That was supposed to be a request changes, sorry.
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.
LGTM
We discussed this in the design meeting and concluded:
declare let t: [string, number]
let n = t.at(-1) // n: number
type TN = (typeof t)[-1] // also number?
// and perhaps even
declare let t2: [...string, number]
let n2 = t.at(-1) Though the second example would require adding quite a bit more code to the checker. I expect @DanielRosenwasser will link the notes when he's done preparing them. |
@rbuckton Can you take another look and merge if all your comments have been addressed? |
It looks like a different PR was merged that already addressed this: #46291 |
Fixes #40695