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

[ReactDOMFactories] Reduce byte size #5759

Conversation

fabiomcosta
Copy link
Contributor

Since the object keys and valus are always the same I decided to transform that to an array.
That should save some bytes from the JS package.
Traversing an array should also be faster than mapObject, which should make this module initialize faster.

@sophiebits
Copy link
Collaborator

This was meant to be safe against Closure advanced mode. Is this actually smaller after gzip?

@fabiomcosta
Copy link
Contributor Author

I don't see how that would not be safe againts GCC advanced mode, but I'll make sure everything works and that the gzip size is better.

@zpao
Copy link
Member

zpao commented Jan 7, 2016

Because app code that does React.DOM.output will get crushed to something like React.DOM.q, but since we use quotes in an array React.DOM.output will actually exist.

@syranide
Copy link
Contributor

syranide commented Jan 8, 2016

@zpao We don't run GCC on React so React.DOM.input would still exist and work, but the element it creates would be <q> or whatever. The other way around.

@zpao
Copy link
Member

zpao commented Jan 8, 2016

Yes, we don't but the goal with the GCC support was so that somebody could if they wanted (eg clojurescript).

@zpao
Copy link
Member

zpao commented Jan 8, 2016

Running "compare_size:files" (compare_size) task
   raw     gz Sizes
  1198    641 build/react-dom-server.js
   734    445 build/react-dom-server.min.js
  1179    632 build/react-dom.js
   715    436 build/react-dom.min.js
699327 157734 build/react-with-addons.js
150302  43356 build/react-with-addons.min.js
632976 142973 build/react.js
137902  39947 build/react.min.js

   raw     gz Compared to master @ 23167f287eda5ee23725a099e67d6086baa8e300
     =      = build/react-dom-server.js
     =      = build/react-dom-server.min.js
     =      = build/react-dom.js
     =      = build/react-dom.min.js
 -2603   -764 build/react-with-addons.js
  -912   -410 build/react-with-addons.min.js
 -2603   -757 build/react.js
  -911   -385 build/react.min.js

mapObject.js (1466 bytes) no longer gets packaged as a result so that's a bit over half the raw savings (which we likely wouldn't see internally). As far as perf goes we're talking about the difference of iterating 130 entries. Since we're all speculating, I estimate the difference is going to be on the order of ns (ie negligible). We could inline mapObject and get > half those bytes back and probably also a speculative perf win (could avoid the .call), while still maintaining GCC compatibility.

@fabiomcosta
Copy link
Contributor Author

@zpao, thx for the data! I've been a bit lazy with this PR.
So what is the conclusion here? It seems to me that it's all positive and we could merge it.

@jimfb
Copy link
Contributor

jimfb commented Jan 9, 2016

@fabiomcosta Did you verify it works with GCC?

@syranide
Copy link
Contributor

syranide commented Jan 9, 2016

AFAIK anyone relying on React.DOM.input and GCC will break, with this you have to do React.DOM['input']. Right? That seems like a bad idea.

@jimfb
Copy link
Contributor

jimfb commented Jan 10, 2016

@syranide Yeah, I agree, we don't want to break that.

@fabiomcosta
Copy link
Contributor Author

@syranide from my understanding this is unrelated to this change. This change won't change the component names from React.DOM and won't make devs use React.DOM['input'].
@jimfb, I'm sending links showing the code compiled with GCC advanced mode. Thx for pushing this, I agree this is important.
[You'll have to select "Advanced" and then press "Compile"]
GCCed with changes: https://goo.gl/rFLcBt
GCCed without changes: https://goo.gl/iVMxMS

@syranide
Copy link
Contributor

@fabiomcosta input specifically is special-cased in GCC and is never mangled, try some of the other tags: https://goo.gl/qf1FWL

@jimfb
Copy link
Contributor

jimfb commented Jan 11, 2016

@fabiomcosta I just looked at the example you posted, I think @syranide is correct, this doesn't minify safely for properties like svg and progress :(.

@fabiomcosta
Copy link
Contributor Author

@syranide @jimfb, cool, I see, I can see now that it does the same for form and some other strings.
But the current code doesn't "solve" that either, I would think that it would make GCC recognize that but the same minification happens to the ReactDOMFactories keys with the current code.
I guess neither the current code nor the new one are safe to be used with GCC advanced mode, as seen by the following link:
https://goo.gl/28mB2z

If the current code wants to be safe with GCC it would have to use the key from the "big object" as the key that is added to React.DOM, but it ignores that and only uses the value.

@sophiebits
Copy link
Collaborator

mapObject preserves keys.

@fabiomcosta
Copy link
Contributor Author

@spicyj, I don't know what I was thinking, you're right. Sorry for the confusion.

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.

6 participants