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

Transform keyMirror to object literal #7505

Closed
wants to merge 1 commit into from
Closed

Transform keyMirror to object literal #7505

wants to merge 1 commit into from

Conversation

zertosh
Copy link
Member

@zertosh zertosh commented Aug 15, 2016

I was hoping that avoiding keyMirror during load would have a more dramatic effect, but I can't tell if it's noise or not. The p90 didn't move, and I'm always reluctant to sell something solely based on averages.

Results using the same benchmark from #7493 with 100 iterations:

engine build before: avg / p90 (ms) after: avg / p90 (ms)
node react.js (development) 22.67 / 23 22.32 / 23
node react.js (production) 20.41 / 21 19.83 / 21
node dist/react-with-addons.js 26.79 / 29 26.20 / 27
node dist/react-with-addons.min.js 18.17 / 19.5 18.03 / 19
node dist/react.js 18.73 / 19 18.02 / 19
node dist/react.min.js 12.87 / 13 12.48 / 13

It does result in some modest size improvements:

   raw     gz Compared to master @ e5f9ae27058052e72d6fb6d89c6ca93075e4089a
     =      = build/react-dom-fiber.js
     =      = build/react-dom-fiber.min.js
  -745    -92 build/react-dom-server.js
  +453     +7 build/react-dom-server.min.js
  -745    -98 build/react-dom.js
  +454     -5 build/react-dom.min.js
 -1322   -337 build/react-with-addons.js
  -166    -39 build/react-with-addons.min.js
 -1322   -339 build/react.js
  -166    -50 build/react.min.js

Given the low-impact of this PR, it won't hurt my feelings if it doesn't get merged.

@vjeux
Copy link
Contributor

vjeux commented Aug 15, 2016

Can you show an example of what the transform does?

const value = prop.get('value');
if (value.isNullLiteral()) {
const name = prop.get('key.name').node;
value.replaceWith(t.stringLiteral(name));
Copy link
Collaborator

@gaearon gaearon Aug 15, 2016

Choose a reason for hiding this comment

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

Correct me if I’m wrong but I think this goes against the purpose of keyMirror.
It exists for Google Closure Compiler property mangling to work.
React is used by Om (and other libraries) from ClojureScript, which in turn relies on GCC.

AFAIK it will break Google Closure Compiler optimizations because

keyMirror({ hello: true })

will get transpiled to

{ hello: 'hello' }

with this plugin (correct me if I’m wrong).

GCC will then mangle this to

{ a: 'hello' }

which is broken.

The intention of keyMirror is that

keyMirror({ hello: true })

gets mangled by GCC to

keyMirror({ a: true })

which will still work (because it produces { a: 'a' }).

@zpao
Copy link
Member

zpao commented Aug 15, 2016

Yea, this will break GCC. We still care about that for now. I think when we decide to stop caring about that, we'll get rid of the use and code style around it, making this pretty moot.

Since you're curious… the other side of this to look at is keyOf, which makes sure we get the same key to lookup for consistent crushing.

@zpao zpao closed this Aug 15, 2016
@zertosh
Copy link
Member Author

zertosh commented Aug 15, 2016

Ah yes, this is not GCC safe 😞

@vjeux
Copy link
Contributor

vjeux commented Aug 17, 2016

@zpao how hard would it be to write a test to make sure that we don't accidentally break GCC?

@zpao
Copy link
Member

zpao commented Aug 18, 2016

Not sure. We'd need to build some minimal app with React, compile it with GCC, and make sure it works. Won't be a jest test. Not sure the extent of the complexity that app would need to actually cover all the cases we might break.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants