Skip to content

Fix emit for shorthand properties when they refer to CommonJS exports. #9151

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
merged 4 commits into from
Jun 14, 2016

Conversation

DanielRosenwasser
Copy link
Member

Fixes #8826 so Yui can be more productive.

@yuit
Copy link
Contributor

yuit commented Jun 14, 2016

@DanielRosenwasser I will not merge it yet as this will have to be completely redo in transform branch. Trying to clean up "transforms" branch right now

const container = resolver.getReferencedExportContainer(node);
return container && container.kind !== SyntaxKind.SourceFile;
return !!container;
Copy link
Contributor

Choose a reason for hiding this comment

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

how can the container not be a sourceFile here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you had a namespace, then the container is probably a namespace (i.e. ModuleDeclaration or ModuleBlock).

Are there any other containers I need to be aware of? Potentially statics on a class?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2016

👍

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jun 14, 2016

@yuit, @mhegazy and I feel that since this is an emit bug and that the fix is small enough, that it can be ported to the transforms branch relatively easily.

@DanielRosenwasser DanielRosenwasser merged commit 8917ddf into master Jun 14, 2016
@DanielRosenwasser DanielRosenwasser deleted the commonjsShorthands branch June 14, 2016 17:32
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 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.

4 participants