Inline spread elements where possible #179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
My use case that leads me to desire this is code conditional on
compile-time constants, e.g. `{ …, ...FEATURE ? { … } : { … } }` (with
Rollup’s tree-shaking resolving the FEATURE ternary before it gets to
Bublé). I first implemented what amounts to this change in Terser
(terser/terser#224), but then realised that I
was running Bublé before Terser, and so Terser never actually *got* the
spreads to inline. The next idea was running Terser, then Bublé, then
Terser again… yuck. Then I decided that this optimisation really
belonged in Bublé anyway: generating less code is good!
Implementation notes:
The ObjectExpression implementation was straightforward and I
implemented it in situ.
Then the ArrayExpression, CallExpression and NewExpression one: at first
I performed the spread inlining in spread()’s while loop, and that
seemed to work well, but I got scared of how ArrayExpression and
CallExpression both handled their “single element” cases specially, and
that they did not appear to be optional inasmuch as removing the
special-casing yielded exceptions in the test suite; so I extracted that
into a separate function. That ended up pleasing me more as well, with
clearly cut semantics and a smaller diff inside the *Expression files,
as they no longer needed to change their single-element special cases to
*not* activate on a SpreadElement containing an ArrayElement.
chris-morgan
force-pushed
the
inline-spread-where-literal
branch
from
8c8c398 to
27728c6
Compare
|
Force push as I remembered I had forgotten to add tests about trailing comma behaviour on object spread. I think I’m finished now. Certainly am for the day. |
|
This seems pretty specific and I feel like it's a bit out-of-scope for bublé, but it's also nice, apparently helps at least one person and is small and self-contained, so I'm merging this. Thanks for your work! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
My use case that leads me to desire this is code conditional on compile-time constants, e.g.
{ …, ...FEATURE ? { … } : { … } }(with Rollup’s tree-shaking resolving the FEATURE ternary before it gets to Bublé). I first implemented what amounts to this change in Terser (terser/terser#224), but then realised that I was running Bublé before Terser, and so Terser never actually got the spreads to inline. The next idea was running Terser, then Bublé, then Terser again… yuck. Then I decided that this optimisation really belonged in Bublé anyway: generating less code is good!Implementation notes:
The ObjectExpression implementation was straightforward and I implemented it in situ.
Then the ArrayExpression, CallExpression and NewExpression one: at first I performed the spread inlining in spread()’s while loop, and that seemed to work well, but I got scared of how ArrayExpression and CallExpression both handled their “single element” cases specially, and that they did not appear to be optional inasmuch as removing the special-casing yielded exceptions in the test suite; so I extracted that
into a separate function. That ended up pleasing me more as well, with clearly cut semantics and a smaller diff inside the *Expression files, as they no longer needed to change their single-element special cases to not activate on a SpreadElement containing an ArrayElement.