Skip to content

Conversation

@GeoffreyBooth
Copy link
Collaborator

Fixes #4629.

An optimization in interpolated strings (which CSX tags essentially are) ignored empty interpolations: "a#{}b" compiled to just `ab`, not `a${}b`. This meant that an interpolation that had nothing but comments, like "a#{### comment ###}b", was also getting output as just `ab`, with the comment lost.

This PR preserves both the optimization and the comments. If an “empty” interpolation has no comments, the current behavior remains. However if it has comments but no other tokens, a zero-length JS token is created in the interpolation to hold the comments; and the optimization isn’t performed.

…hat contain nothing but comments should not be optimized out
@GeoffreyBooth GeoffreyBooth requested a review from lydell August 24, 2017 01:36
Copy link
Collaborator

@lydell lydell left a comment

Choose a reason for hiding this comment

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

This looks good for the JSX case! 👍

Unfortunately, "a#{###test###}b" becomes
`a${/*test*/}b`, which is invalid JS.

@GeoffreyBooth
Copy link
Collaborator Author

Huh. I had no idea `a${/*test*/}b` is invalid JS. How weird! I wonder why?

So what should we do for strings? Compile to `a${/*test*/''}b`?

@lydell
Copy link
Collaborator

lydell commented Aug 24, 2017

Empty interpolations are invalid in JS. I don't know why but that's how it is. I think your solution with the empty string looks good.

…if we only have a comment to put in there, toss in an empty string as well
@GeoffreyBooth
Copy link
Collaborator Author

Done!

@GeoffreyBooth GeoffreyBooth merged commit 40c3511 into jashkenas:2 Aug 24, 2017
@GeoffreyBooth GeoffreyBooth deleted the interpolations-with-only-comments branch August 24, 2017 06:35
@connec
Copy link
Collaborator

connec commented Aug 24, 2017

We should probably make "a#{###test###}b" invalid CS.

@lydell
Copy link
Collaborator

lydell commented Aug 24, 2017

@connec You mean empty interpolations in general?

@connec
Copy link
Collaborator

connec commented Aug 24, 2017

Yeh, I don't think it's useful to allow empty interpolations.

That said, I think I misunderstood the outcome of the PR - treating empty interpolations as '' seems reasonable too.

@GeoffreyBooth
Copy link
Collaborator Author

It would be a breaking change to no longer allow "a#{###test###}b", not that that would be so terrible. But I think since the solution is so simple, we might as well improve on template literals a little 😄

@GeoffreyBooth GeoffreyBooth mentioned this pull request Sep 2, 2017
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.

3 participants