bublejs / buble

https://buble.surge.sh
MIT License
871 stars 64 forks source link

Inline spread elements where possible #179

Closed chris-morgan closed 5 years ago

chris-morgan commented 5 years ago

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 (https://github.com/terser-js/terser/pull/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 commented 5 years ago

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.

adrianheine commented 5 years ago

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!