GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add static index signature by Kingwl · Pull Request #37797 · microsoft/TypeScript Conversation
| |||||||||||||||||||||||||||||||||||||||||||||||||
|
TypeScript Bot (@typescript-bot) pack this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at a5f3d15. You can monitor the build here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
TypeScript Bot (@typescript-bot) pack this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Heya Orta Therox (@orta), I've started to run the tarball bundle task on this PR at a5f3d15. You can monitor the build here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Hey Orta Therox (@orta), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Wesley Wigham (@weswigham) the reviewer list is broken on this PR, but you should look at this too |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
⬆️ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| } | ||
| } | ||
|
|
||
| function getIndexInfoOfIndexSymbol(indexSymbol: Symbol, indexKind: IndexKind) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Since this is used only once in the above changed code, maybe make it a local helper instead?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I kinda prefer it this way to avoid several closure allocations, but it's likely a premature optimization.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
(I'd be surprised if at least the v8 compiler wouldn't optimize it away...)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -11094,24 +11114,29 @@ namespace ts { | |||
| } | |||
|
|
|||
| function getIndexSymbol(symbol: Symbol): Symbol | undefined { | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
You're splitting both getIndexSymbol and getIndexDeclarationOfSymbol to two functions each, because your code calls them with different inputs, and it's the only place that needs the two new entry points. One such refactor seems questionable, but two looks like too much. (But OTOH I can't figure out if there's a way to avoid it.)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ping Daniel Rosenwasser (@DanielRosenwasser), Wesley Wigham (@weswigham). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
7e5091d to
6d8ed7b
Compare
|
Hi, Folks, I don't know what I can do. Should I work with the parser issue inside this PR? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I would say no - though, if you want to tackle the problem, coordinate with Ron Buckton (@rbuckton) on #38283. Otherwise, I'm not familiar enough with the internals. Eli Barzilay (@elibarzilay) and Wesley Wigham (@weswigham), can you give this another review? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good, but I'd love to see some more tests (you should consider placing them in a subfolder of the conformance directory instead of compiler, since it's a "new" language feature). Specifically, I'm thinking of tests showing:
(typeof Cls)[string]keyof, eg keyof typeof ClsExtract<typeof Cls, string>More would also be welcome, ofc, if you can think of any other possible edge cases.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
2d61217 to
d4624e6
Compare
|
Seems something wrong if extends others |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
TypeScript Bot (@typescript-bot) pack this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at 109e7c3. You can monitor the build here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Eli Barzilay (@elibarzilay) Wesley Wigham (@weswigham) I'd like to get this in early in the 4.3 cycle. Does this (1) have enough tests now (2) error correctly on property types? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Friendly ping 👀 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
👀PiNg AgAiN |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Assuming my question is answered, I think this is probably good now, with Nathan Shively-Sanders (@sandersn) 's questions both answering in the affirmative. Eli Barzilay (@elibarzilay) since you're the PR assignee, you can probably merge this for this cycle whenever you're ready.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
just for verify TypeScript Bot (@typescript-bot) pack this. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at f5508e1. You can monitor the build here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hrm, I'm not certain why the playground didn't build (as this seems to be completely up to date with master), I'll look into add some more logs in the build process. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Try again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at f5508e1. You can monitor the build here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Allow defining static index signature
There was an error while loading. Please reload this page.