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

Fix #4774: export default followed by an object should always work #4783

Merged

Conversation

GeoffreyBooth
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth commented Nov 12, 2017

Fix #4774: export default followed by an object should always work, even if the object contains braces (or put another way, even if one of the object’s values is an explicit object, defined with braces).

Fixes a bug introduced in #4532, where export default was given the ability to export an implicit object. In that PR, this was implemented in an incorrect way: default followed by a newline caused a suppression of that newline. For objects that contain explicit objects (i.e. braces), this breaks, as the suppressed newline screws up the tracking of indents, which matters when we encounter } tokens. Instead we should handle it in the grammar the same way returning an implicit object is handled.

@connec you had some comments on the earlier PR. @helixbass you did some work related to this in #4605.

…ays work, even if the object contains braces. `default` shouldn't suppress a newline, we should handle it in the grammar the same way returning an implicit object is handled
@GeoffreyBooth GeoffreyBooth merged commit 637fe30 into jashkenas:master Nov 16, 2017
@GeoffreyBooth GeoffreyBooth deleted the fix-export-default-object branch November 16, 2017 05:38
@helixbass
Copy link
Collaborator

@GeoffreyBooth just got notified now (post-merge) - lgtm, by explicitly restricting it to an indented Object in the grammar you're also getting comparable benefits to #4605 (for return) where unintended (ie non-object) indented cases are disallowed eg:

export default
  foo

@GeoffreyBooth
Copy link
Collaborator Author

Awesome, thanks.

@GeoffreyBooth
Copy link
Collaborator Author

@helixbass if you have time, I'd love some review of the other recent open pull requests 😄

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.

2 participants