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
Optimize computed properties output (byte-wise) #6652
Optimize computed properties output (byte-wise) #6652
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/55254/ |
var heh = "heh"; | ||
var noo = "noo"; | ||
var foo = "foo"; | ||
|
||
var obj = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how it worked before, either foo
and bar
were undefined in the score or you now re-define them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was purely a fixture/snapshot test, I've added also exec.js
and that's why I had to add those to the scope in actual/expected fixtures (like you have noticed - they were previously undefined).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that make sense
@Andarist do we stil want this change? |
I would be in favour of merging this (after rebase ofc), but unfortunately noone (except you) has reviewed this. |
My review wasn't very useful, to push this forward we could discuss about it during our next meeting. Could you please add it to our agenda here babel/notes#73. thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what the actual byte difference ends up being once GZip is taken into account?
Is it worth exploring a new helper to optimize this instead? I don't know if that would be harder to keep the property evaluation ordering.
var heh = "heh"; | ||
var noo = "noo"; | ||
var foo = "foo"; | ||
var obj = babelHelpers.defineProperty(babelHelpers.defineProperty(babelHelpers.defineProperty(babelHelpers.defineProperty(babelHelpers.defineProperty({}, "x" + heh, "heh"), "y" + noo, "noo"), foo, "foo1"), "foo", "foo2"), "bar", "bar"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we go with this, we should probably have a cap on the nesting here, or else we risk objects with lots of keys creating extremely deep AST structures.
07fd689
to
e2375b7
Compare
@JLHwung @liuxingbaoyu This is something that we can still do. I rebased this PR, and added a cap as suggested in #6652 (comment). |
e2375b7
to
ff49c4e
Compare
Landing this PR was not my 2023 bingo card… 😝 |
Life is full of surprises 😛 |
ff49c4e
to
18c83f2
Compare
It is a small "optimization" of the computed properties output. The change basically wraps (if possible) previous element -
initProps
object or previousdefineProperty
call when "pushing" newdefineProperty
calls.It leverages the fact that
defineProperty
returns the passed object, therefore in most cases we do not need to create temporary references and stuff.babel-plugin-transform-computed-properties/test/fixtures/spec/multiple illustrates this nicely.
Before
After
Not only it saves some bytes in the output, it is probably more friendly for the minifiers too.