-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Allow special element access assignments to create declarations #33537
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
Allow special element access assignments to create declarations #33537
Conversation
tests/cases/fourslash/referencesForStringLiteralPropertyNames6.ts
Outdated
Show resolved
Hide resolved
tests/cases/fourslash/referencesForStringLiteralPropertyNames7.ts
Outdated
Show resolved
Hide resolved
@andrewbranch I suspect this overlaps with #33220 quite a bit, if not is wholly consumed by it |
@weswigham I just checked out your PR, and In my branch, |
Ok, now my PR works with |
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 might look into the spacing changes before merging this.
You wanna merge with master and see what some of these (class members, especially) emit with |
Interestingly enough, the late bound variant in the other PR is already in a good place wrt declarations, looks like: https://github.com/microsoft/TypeScript/pull/33220/files#diff-2157e3c82025f7ab3e125ece2a9007eb I'll try to get that merged so we can reconcile and work on the changes to the declaration emitter - |
I think this will be for 3.8, as #33220 is still open—I won’t have time to fix conflicts, much less fix the JS declaration emit. |
Naw, I think this'll maybe still be in 3.7 - #33220 will be merged once the new test shows as passing and because of other things, the 3.7 beta snap is being delayed a bit. |
@weswigham this is fixed up with your PR now, just need to work on the declaration emit Monday morning 👍 |
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.
A nav bar test like tests/cases/fourslash/navigationBarItemsSymbols4.ts
is probably pertinent. If only to comment that none of these show up in the navbar because they're all non-syntactic members.
Otherwise, just some nits that could be cleaned up.
They actually do show up. I updated that test and it exposed one more place in navigationBar that was checking exclusively for PropertyAccessExpression. Ready to merge once tests run 👍 |
Fixes #32153
Allows expando properties to be created via element access: