Skip to content
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 variable statements used as declaration sites to be marked visible #22798

Merged

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Mar 22, 2018

And included in declaration emit by alias marking.

Fixes #20080 (replacing #20169)

@weswigham weswigham requested review from sandersn, mhegazy and a user March 22, 2018 18:38
@weswigham weswigham force-pushed the mark-unexported-vars-visible-for-names branch from 917d939 to d5179ae Compare March 22, 2018 21:03
isDeclarationVisible(anyImportSyntax.parent)) {
return addVisibleAlias(declaration, anyImportSyntax);
}
else if (isVariableDeclaration(declaration) && isVariableStatement(declaration.parent.parent) &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not we do this for all declarations and not jsut variable declarations? e.g. #23127

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So allow private interfaces, types, anything - to be marked and emitted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are variables any special? they are all declarations..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just because they're only used to name unique symbols. I can update to handle the other kinds of declaration statements, but it'll require more work to light up each private declaration kind in the declaration emitter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u want to get this in first then do the other ones? that is fine by me as well.. but we need to do them all, no point in half doing it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also did @mhegazy's ask included making it visible all the time.. I thought it implied only in export = case i think, because other wise we would be making lot of stuff visible which might not be intended?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

he added code later on to make it emit an extra export {}; at the bottom, this should change the module scoping.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sheetalkamat The aliases that get returned by this function are the statements the emitter needs to revisit - they are not blindly emitted. The visibility is actually handled by setting isVisible on the node links (done in addVisibleAlias), which is only done on the specific variable declaration that is the symbol's source.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test wherein only one variable declaration becomes visible. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weswigham weswigham merged commit 83ab341 into microsoft:master Apr 9, 2018
@weswigham weswigham deleted the mark-unexported-vars-visible-for-names branch April 9, 2018 21:30
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants