GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add static index signature by Kingwl · Pull Request #37797 · microsoft/TypeScript

Conversation

@Kingwl

@Kingwl Wenlu Wang (Kingwl) commented Apr 5, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Fixes #6480

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ExE-Boss, wsmd, WillAvudim, nokazn, biocomputer924, and Enteleform reacted with thumbs up emoji bajtos, WillAvudim, and Enteleform reacted with heart emoji ExE-Boss, wsmd, brianmhunt, WillAvudim, and Enteleform reacted with rocket emoji
All reactions
  • 👍 6 reactions
  • ❤️ 3 reactions
  • 🚀 5 reactions
@Kingwl
Wenlu Wang (Kingwl) force-pushed the static_index branch from f4f354c to a5f3d15 Compare April 5, 2020 05:52
@Kingwl

Wenlu Wang (Kingwl) commented Apr 5, 2020

Copy link
Copy Markdown
Contributor Author

TypeScript Bot (@typescript-bot) pack this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 5, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at a5f3d15. You can monitor the build here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 5, 2020

Copy link
Copy Markdown
Contributor

Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/70462/artifacts?artifactName=tgz&fileId=3AF6CC9DE1A8ED4CFFF5BB83EE19491B3EC20CE91E716546D47CA7344C97A65702&fileName=/typescript-3.9.0-insiders.20200405.tgz"
    }
}

and then running npm install.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Kingwl Wenlu Wang (Kingwl) changed the title Add static index Add static index signature Apr 6, 2020
@orta

Orta Therox (orta) commented Apr 8, 2020

Copy link
Copy Markdown
Contributor

TypeScript Bot (@typescript-bot) pack this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 8, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Orta Therox (@orta), I've started to run the tarball bundle task on this PR at a5f3d15. You can monitor the build here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 8, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hey Orta Therox (@orta), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/70679/artifacts?artifactName=tgz&fileId=A68270BE90993FCC02761F5C7DFC4E733CB522BA025C69FCE66DDE502EC10C9B02&fileName=/typescript-3.9.0-insiders.20200408.tgz"
    }
}

and then running npm install.


There is also a playground for this build.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sandersn Nathan Shively-Sanders (sandersn) added the For Backlog Bug PRs that fix a backlog bug label Apr 15, 2020
@sandersn Nathan Shively-Sanders (sandersn) assigned Eli Barzilay (elibarzilay) Apr 15, 2020
@sandersn

Nathan Shively-Sanders (sandersn) commented Apr 15, 2020

Copy link
Copy Markdown
Member

Wesley Wigham (@weswigham) the reviewer list is broken on this PR, but you should look at this too

Kingwl reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Kingwl

Wenlu Wang (Kingwl) commented Apr 26, 2020

Copy link
Copy Markdown
Contributor Author

⬆️

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

elibarzilay
Eli Barzilay (elibarzilay) suggested changes Apr 29, 2020
View reviewed changes
Comment thread tests/baselines/reference/staticIndexer.errors.txt Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread tests/baselines/reference/staticIndexers.errors.txt Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread tests/baselines/reference/staticAsIdentifier.errors.txt Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/compiler/checker.ts
}
}

function getIndexInfoOfIndexSymbol(indexSymbol: Symbol, indexKind: IndexKind) {

@elibarzilay Eli Barzilay (elibarzilay) Apr 29, 2020

Copy link
Copy Markdown
Contributor

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.

Since this is used only once in the above changed code, maybe make it a local helper instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Apr 29, 2020

Copy link
Copy Markdown
Member

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 kinda prefer it this way to avoid several closure allocations, but it's likely a premature optimization.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@elibarzilay Eli Barzilay (elibarzilay) Apr 29, 2020

Copy link
Copy Markdown
Contributor

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'd be surprised if at least the v8 compiler wouldn't optimize it away...)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/compiler/checker.ts
@@ -11094,24 +11114,29 @@ namespace ts {
}

function getIndexSymbol(symbol: Symbol): Symbol | undefined {

@elibarzilay Eli Barzilay (elibarzilay) Apr 29, 2020

Copy link
Copy Markdown
Contributor

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@elibarzilay
Eli Barzilay (elibarzilay) requested a review from Wesley Wigham (weswigham) April 29, 2020 20:29
@elibarzilay

Eli Barzilay (elibarzilay) commented Apr 29, 2020

Copy link
Copy Markdown
Contributor

Ping Daniel Rosenwasser (@DanielRosenwasser), Wesley Wigham (@weswigham).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested changes Apr 29, 2020
View reviewed changes
Comment thread tests/baselines/reference/staticAsIdentifier.errors.txt Resolved

Uh oh!

There was an error while loading. Please reload this page.

@Kingwl
Wenlu Wang (Kingwl) force-pushed the static_index branch from 7e5091d to 6d8ed7b Compare April 30, 2020 09:33
@Kingwl

Wenlu Wang (Kingwl) commented May 19, 2020

Copy link
Copy Markdown
Contributor Author

Hi, Folks, I don't know what I can do. Should I work with the parser issue inside this PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented May 19, 2020

Copy link
Copy Markdown
Member

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

weswigham
Wesley Wigham (weswigham) requested changes May 20, 2020
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

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.

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:

  • Class static inheritance both inheriting and overriding index signatures (and issuing errors if you override with an incompatible type)
  • Assignability of class static sides to one another when index signatures are in place (readonly and not readonly)
  • Assignability between class static sides and equivalent interfaces describing them
  • Assignability between class static sides with generic static index signatures (this likely requires defining classes within a closure with generic parameters) to have some sanity checks on the behavior of variance digests with these
  • Some usages with indexed access types, eg (typeof Cls)[string]
  • Some usages with keyof, eg keyof typeof Cls
  • Some usages with homomorphic mapped types, eg Extract<typeof Cls, string>

More would also be welcome, ofc, if you can think of any other possible edge cases.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Wenlu Wang (Kingwl) added 6 commits May 20, 2020 21:34
@Kingwl
Add static index
ed1a798
@Kingwl
fix lint
f83b7de
@Kingwl
make lint happy
d6fdab8
@Kingwl
adjust test cases
8eca2be
@Kingwl
add more cases
b7f614f
@Kingwl
fix changes
d4624e6
@Kingwl
Wenlu Wang (Kingwl) force-pushed the static_index branch from 2d61217 to d4624e6 Compare May 20, 2020 13:36
Wenlu Wang (Kingwl) added 2 commits May 20, 2020 21:51
@Kingwl
Add more case
a13190a
@Kingwl
accept baseline
52884cc
@Kingwl

Wenlu Wang (Kingwl) commented May 20, 2020

Copy link
Copy Markdown
Contributor Author

Seems something wrong if extends others

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Kingwl

Wenlu Wang (Kingwl) commented Jan 19, 2021

Copy link
Copy Markdown
Contributor Author

TypeScript Bot (@typescript-bot) pack this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 19, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at 109e7c3. You can monitor the build here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Wenlu Wang (Kingwl) added 3 commits January 20, 2021 00:12
@Kingwl
fix crash
0ee41f6
@Kingwl
Accept baseline
f91d84b
@Kingwl
Merge branch 'master' into static_index
aaf5185
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Feb 11, 2021
TypeScript 4.3 Iteration Plan #42762
Closed
@sandersn

Nathan Shively-Sanders (sandersn) commented Feb 16, 2021

Copy link
Copy Markdown
Member

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?

Kingwl reacted with rocket emoji Kingwl reacted with eyes emoji
All reactions
  • 🚀 1 reaction
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Wenlu Wang (Kingwl) added 3 commits March 4, 2021 13:01
@Kingwl
Merge branch 'master' into static_index
f8eddee
@Kingwl
Fix regression
246585f
@Kingwl
Fix crash
124e461
@Kingwl

Wenlu Wang (Kingwl) commented Mar 4, 2021

Copy link
Copy Markdown
Contributor Author

Friendly ping 👀

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) added the Rescheduled This issue was previously scheduled to an earlier milestone label Mar 4, 2021
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) modified the milestones: TypeScript 4.2.0, TypeScript 4.3.0 Mar 4, 2021
@Kingwl

Wenlu Wang (Kingwl) commented Mar 25, 2021

Copy link
Copy Markdown
Contributor Author

👀PiNg AgAiN

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

weswigham
Wesley Wigham (weswigham) approved these changes Mar 25, 2021
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

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

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/compiler/checker.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Wenlu Wang (Kingwl) added 2 commits March 25, 2021 16:12
@Kingwl
Merge branch 'master' into static_index
23cbc31
@Kingwl
always return new array
f5508e1
@Kingwl

Wenlu Wang (Kingwl) commented Mar 25, 2021

Copy link
Copy Markdown
Contributor Author

just for verify

TypeScript Bot (@typescript-bot) pack this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 25, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at f5508e1. You can monitor the build here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 25, 2021

Copy link
Copy Markdown
Contributor

Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/99257/artifacts?artifactName=tgz&fileId=E1B419FD818D7F5E2314236B7CDD646F933C3C20811250634D11FB9268A194FA02&fileName=/typescript-4.3.0-insiders.20210325.tgz"
    }
}

and then running npm install.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@orta

Orta Therox (orta) commented Mar 25, 2021

Copy link
Copy Markdown
Contributor

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.

Kingwl reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Kingwl

Wenlu Wang (Kingwl) commented Mar 26, 2021

Copy link
Copy Markdown
Contributor Author

Try again.
TypeScript Bot (@typescript-bot) pack this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 26, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Wenlu Wang (@Kingwl), I've started to run the tarball bundle task on this PR at f5508e1. You can monitor the build here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@typescript-bot

TypeScript Bot (typescript-bot) commented Mar 26, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Hey Wenlu Wang (@Kingwl), I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/99342/artifacts?artifactName=tgz&fileId=9F9DC5C2BFBC2F804A9BB87AAC7535FF4F449685817397BB01F4B3AB0D61A51C02&fileName=/typescript-4.3.0-insiders.20210326.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Kingwl
Wenlu Wang (Kingwl) requested a review from Daniel Rosenwasser (DanielRosenwasser) March 26, 2021 06:55
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) approved these changes Mar 26, 2021
View reviewed changes
@DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) merged commit 41dc625 into microsoft:master Mar 26, 2021
@Kingwl
Wenlu Wang (Kingwl) deleted the static_index branch June 30, 2021 08:27
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser approved these changes
@weswigham Wesley Wigham (weswigham) weswigham approved these changes
+1 more reviewer
@elibarzilay Eli Barzilay (elibarzilay) elibarzilay approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@elibarzilay Eli Barzilay (elibarzilay)

Labels

For Backlog Bug PRs that fix a backlog bug Rescheduled This issue was previously scheduled to an earlier milestone

Projects

PR Backlog
Archived in project

Milestone

TypeScript 4.3.0

Development

Successfully merging this pull request may close these issues.

Allow defining static index signature

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@Kingwl @typescript-bot @orta @sandersn @elibarzilay @DanielRosenwasser @weswigham @RyanCavanaugh
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.