Skip to content

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

Merged

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Sep 20, 2019

Fixes #32153

Allows expando properties to be created via element access:

function Paragraph(props) {
  return <p>{props.children}</p>;
}

Paragraph.displayName = "Paragraph"; // This has worked for a while
Paragraph["defaultProps"] = {}; // This PR enables this

@andrewbranch andrewbranch marked this pull request as ready for review September 23, 2019 22:47
@weswigham
Copy link
Member

@andrewbranch I suspect this overlaps with #33220 quite a bit, if not is wholly consumed by it

@sandersn sandersn self-assigned this Sep 24, 2019
@andrewbranch
Copy link
Member Author

@weswigham I just checked out your PR, and the only place I can observe overlap is on this["str"] = something I actually observe no overlap, because I have a mistake on this["str"] = something

image

In my branch, all of these work (though the this assignments trigger implicit any errors, as they do with property access assignments) all of these work except for the this assignments, but they’re supposed to.

@andrewbranch
Copy link
Member Author

Ok, now my PR works with this, and that’s the place we overlap.

Copy link
Member

@sandersn sandersn left a 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.

@weswigham
Copy link
Member

You wanna merge with master and see what some of these (class members, especially) emit with declarations on? Since they're not latebound, I imagine they're just emitted as their names, but tests are nice (especially of names like "prop-with-dashes").

@andrewbranch
Copy link
Member Author

Oh dear

image

@weswigham
Copy link
Member

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 -

@andrewbranch
Copy link
Member Author

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.

@weswigham
Copy link
Member

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.

@andrewbranch
Copy link
Member Author

@weswigham this is fixed up with your PR now, just need to work on the declaration emit Monday morning 👍

@andrewbranch andrewbranch added this to the TypeScript 3.7.0 milestone Sep 30, 2019
Copy link
Member

@weswigham weswigham left a 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.

@andrewbranch
Copy link
Member Author

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.

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JS IntelliSense fails to treat exports["default"] the same as exports.default
4 participants