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

Reduced size of JSXTransformer.js by 10%. #1995

Closed
wants to merge 1 commit into from

Conversation

RReverser
Copy link
Contributor

Decided to add this small part from #1234 that uses native browser's btoa function for Base64 encoding and so allows Browserify not to include browser version of Node.js buffer module.

Comparison table:

raw gz
before 421 641 84 774
after 384 700 76 649
diff 36 941 8 125
diff (%) 9% -10%

             |     raw |       gz
-------------|---------|---------
**before**   | 421 641 |   84 774
**after**    | 384 700 |   76 649
**diff**     |  36 941 |    8 125
**diff (%)** |      9% |     -10%
@zpao
Copy link
Member

zpao commented Aug 4, 2014

This needs to also work in non-browser environments, at least for the time being. react-rails, react-python (React.net too I think) use this file to parse JSX in basically raw insert JS engine processes. We could build & ship a different file for those cases, though I'm not sure how much we really care. 10% is a nice win but it's a dev tool for browsers so I don't think it needs to be terribly optimized.

@RReverser
Copy link
Contributor Author

This needs to also work in non-browser environments, at least for the time being. react-rails, react-python (React.net too I think) use this file to parse JSX in basically raw insert JS engine processes.

Why don't those projects use react-tools instead?

10% is a nice win but it's a dev tool for browsers so I don't think it needs to be terribly optimized.

I don't think removing extra dependency and changing couple of lines can be called "terrible optimization". This is rather similar to omitting jQuery dependency in page that just needs to animate menu.

@zpao
Copy link
Member

zpao commented Aug 4, 2014

Why don't those projects use react-tools instead?

Because we don't have a react-tools bundled file. It needs to be a standalone bundle, not an npm package. We could do that, and I'm thinking of getting rid of JSXTransformer in its current form and just doing that (see #1966). JSXTransformer currently happens to work just fine in both places so there's been no driving force.

Also, I meant "terribly optimized" in the best way. s/terribly/very/ and it's probably a better sentence. Right now the benefits outweigh the additional 10% size. I mean, we could also ship a minimized version (#1855) and shave off more, but we don't because it shouldn't be used for production where these things matter.

Let's talk in #1966 about what we can do for a possible ReactTools.js. In the meantime, I'm not going to make a change like this to JSXTransformer as it exists. I appreciate you making this and starting the discussion though!

@zpao zpao closed this Aug 4, 2014
@RReverser
Copy link
Contributor Author

Also, I meant "terribly optimized" in the best way. s/terribly/very/ and it's probably a better sentence.

Don't worry, I did understand it in the same way and responded accordingly. And I do understand and agree with your arguments.

@RReverser
Copy link
Contributor Author

@zpao Btw, I was also going to implement piece of functionality for #1913 / #1977 - is it irrelevant now, too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants