Switched from browserify to pure-cjs bundler. #1234

RReverser · 2014-03-08T15:25:06Z

Rebase of #1002.

Optimizations and fix for JSXTransformer build. Dropped dependency on emulation of Node.js native modules. Added deamdify step for JSXTransformer build.

zpao · 2014-03-10T23:22:23Z

I didn't really get to give this a close look last time around so a couple things:

Minification step (grunt build:min) takes forever and a day. Any idea why? To the point where grunt build is unusable for actual work.
We should really rename these files from browserify if we aren't using it (you mentioned last time it wasn't the interesting part, but once we get this ready 2nd commit to move them would be nice).
What do we need deamdify for? I don't think we have anything with the AMD prelude.
Will this have stable sorting? One problem with browserify early on was that require path building was async and resulting in the build file being nondeterministic, which was a non-trivial pain.
Your UMD is slightly different from the one browserify uses (https://github.com/ForbesLindesay/umd/blob/master/template.js), namely the part where yours doesn't check for other globals, only window. Any foreseeable problems?
All of the intermediate license headers are getting stripped out. That should be fine but we'll want to stop using simpleBannerify But I ask because I wanted to know the reasoning.

And to bring in the table you had last time:

orig	orig.gz	diff	diff.gz	filename	diff %	diff.gz %
392755	78327	-26789	-20464	JSXTransformer.js	-7%	-26%
602322	123001	-4635	-1843	react-with-addons.js	-1%	-1%
121441	33430	-17999	-3576	react-with-addons.min.js	-15%	-11%
549027	112014	-2098	-1096	react.js	-4%	-1%
112340	31015	-16826	-3389	react.min.js	-15%	-11%

…0 for better build speed.

RReverser · 2014-03-11T11:51:05Z

Minification step (grunt build:min) takes forever and a day. Any idea why? To the point where grunt build is unusable for actual work.

Ugh, again... Sorry, was already discussed with @benjamn and fixed previous time but lost in merge conflicts after rebase. Fixed it back. And added pure-cjs@1.9.0 which gives even better build speed by not caring about original formatting and comments.
We should really rename these files from browserify if we aren't using it (you mentioned last time it wasn't the interesting part, but once we get this ready 2nd commit to move them would be nice).

Done.
What do we need deamdify for? I don't think we have anything with the AMD prelude.

That was needed for JSXTransformer which uses source-map which uses AMD Simplified CommonJS Wrapper with amdefine for the inner structuring and ability to be used in browser (you may want to look at, for example, https://github.com/mozilla/source-map/blob/master/lib/source-map/binary-search.js). This caused amdefine to be included into the bundle and used instead of using native CommonJS form which IS actually already used inside define callback, so I decided to add this transformation step.
Will this have stable sorting? One problem with browserify early on was that require path building was async and resulting in the build file being nondeterministic, which was a non-trivial pain.

Sorry, I don't really get the question.
Your UMD is slightly different from the one browserify uses (https://github.com/ForbesLindesay/umd/blob/master/template.js), namely the part where yours doesn't check for other globals, only window. Any foreseeable problems?

I don't see, do you? My UMD is pretty similar to https://github.com/umdjs/umd/blob/master/returnExportsGlobal.js, except that it does not use any external dependencies (yet) so could be simplified. The only global we should care about when AMD and CommonJS are not available is browser's window, so I don't see any problems on how it should be referrred. However, this inside anonymous function in any JS engine refers to global object, so this should be fine in any case.
All of the intermediate license headers are getting stripped out. That should be fine but we'll want to stop using simpleBannerify But I ask because I wanted to know the reasoning.

pure-cjs uses AST manipulations for advanced optimizations instead of strings transformations, so comments, formatting and so on are not preserved since they are obviously not part of AST.

…ility.

benjamn · 2014-03-17T14:30:58Z

Gruntfile.js

-  // Our own browserify-based tasks to build a single JS file build
-  grunt.registerMultiTask('browserify', browserifyTask);
+  // Our own commonjs-based tasks to build a single JS file build
+  grunt.registerMultiTask('commonjs', commonjsTask);


I would prefer to call this task bundle instead of commonjs, because we're bundling CommonJS modules together, and bundle is a generic term that makes sense regardless of which particular bundling tool we use.

Ok, I can change that easily.

Done in 3f3187c.

Switched from browserify to pure-cjs bundler.

zpao · 2014-03-18T17:10:13Z

we'll want to stop using simpleBannerify

This never happened. I'll fix it.

I'm going to live with this for a bit. I'm shipping 0.10 very soon so this will go out there. The file size wins are nice, but I'm actually not happy with the code that's generated. Stripping out comments from the dev build might actually be a deal breaker (especially as we encourage people to use their debugger and step through code).

zpao · 2014-03-18T17:11:04Z

Oh and the license header says "undefined" :/

RReverser · 2014-03-18T17:19:38Z

Let me look into license header problem. Regarding comments - we could use recast for saving them in dev version, but ideally I rather want to work on enabling source maps through entire build process (they are lost now due to few plugins that don't support them).

RReverser · 2014-03-18T17:39:43Z

License header fixed by #1267.
And let me know what you think about [built dev version with comments] vs [source maps to real original files].

zpao · 2014-03-18T17:59:31Z

That solves part of the problem. We can't ship a file without a license header and the non-min builds don't include the apache stuff. I will fix this.

BUT The other big concern and is actually a dealbreaker - grunt build run twice in a row creates different files. The size compare shows random +/- (not consistently the same files). I can't have this in releases. If I checkout a release branch with the same versions of all files, I should be able to generate the exact same build.

That's what I meant when I said this:

One problem with browserify early on was that require path building was async and resulting in the build file being nondeterministic

RReverser · 2014-03-18T18:09:07Z

That solves part of the problem. We can't ship a file without a license header and the non-min builds don't include the apache stuff. I will fix this.

For me changing license texts to different ones just didn't seem to be part of scope of this task.

That's what I meant when I said this

Got it. But could you reproduce it with pure-cjs? I'm actually persisting ids exactly for saving order in https://github.com/RReverser/pure-cjs/blob/master/lib/replacerMap.js#L15-L22, ideally random reordering should never happen here (and I can't reproduce it locally, at least sizes are always the same).

zpao · 2014-03-18T18:14:46Z

Ah, well when you removed the license header from all of the parts, we need to add it to the whole.

I'm reproducing with master right now. This is a diff between 2 runs. https://gist.github.com/zpao/9625952

RReverser · 2014-03-18T18:20:55Z

Ah, well when you removed the license header from all of the parts, we need to add it to the whole.

Ah, so you were talking about Apache headers... missed that in previous comment. That's right. From other point, it might be even better when you have one license comment for bundled file instead of a lot duplicates for every module function.

I'm reproducing with master right now. This is a diff between 2 runs.

Nice. (not really)

Should be easy to fix in dirty way, but just wondering - why is that critical for you? I mean, modules are working in the same way in any case.

zpao · 2014-03-18T18:23:23Z

If I checkout a release branch with the same versions of all files, I should be able to generate the exact same build.

zpao · 2014-03-18T18:23:47Z

I'm backing this out until the problem can be resolved. Sorry

RReverser · 2014-03-18T18:25:21Z

If I checkout a release branch with the same versions of all files, I should be able to generate the exact same build.

Yeah, and that's what I'm wondering about - why is it critical to generate exactly same bundled file if it doesn't change any logic, only order of elements in array?

zpao · 2014-03-18T18:31:31Z

Because I want my life to be simple. When I build in a branch and get a different version of a file, is it because a change was introduced? Or did my build system change the way it worked? I don't want to diff and figure that out.

The other part of this (but building on simple) is ensuring that all files we distribute are identical. The file that ends up on bower, on the cdn, on cdnjs, in our starter kit, etc. If I build once and generate most of that and then happen to run the build again, I've just made a bunch of work for myself.

If somebody else checks out the branch and builds him/herself, and needs to track down a line number from an error but it's different from other people's, that's annoying.

So maybe that sounds silly to you but deterministic builds are currently a requirement for me.

RReverser · 2014-03-18T18:42:53Z

So maybe that sounds silly to you

Now it doesn't. Don't worry, I just needed some explanations since head doesn't work that good in the evening :P

Working on that right now. Could you please not to revert it for now so I wouldn't have to do one more rebase tomorrow?

This reverts commit 7987e6a, reversing changes made to d88d479.

zpao · 2014-03-18T18:46:04Z

I'm shipping 0.10rc today so no. But you can do a fancy unrevert and we'll see what we can do for 0.11

RReverser · 2014-03-18T19:32:08Z

Fixed in RReverser/react@b6519dd + RReverser/pure-cjs@5a1fe63. Are you still interested?

Switched from browserify to pure-cjs bundler.

3171436

Optimizations and fix for JSXTransformer build. Dropped dependency on emulation of Node.js native modules. Added deamdify step for JSXTransformer build.

zpao added the build/tooling label Mar 10, 2014

Renamed browserify tasks/configs to cjs, updated pure-cjs to 1.9.…

9e224e6

…0 for better build speed.

CommonJS builder config to Grunt style for easier handling and readab…

25773ed

…ility.

benjamn reviewed Mar 17, 2014
View reviewed changes

Renamed commojs task to bundle, added failing build if bundler failed.

3f3187c

benjamn added a commit that referenced this pull request Mar 18, 2014

Merge pull request #1234 from RReverser/pure-cjs

7987e6a

Switched from browserify to pure-cjs bundler.

benjamn merged commit 7987e6a into facebook:master Mar 18, 2014

zpao added a commit that referenced this pull request Mar 18, 2014

Revert "Merge pull request #1234 from RReverser/pure-cjs"

8d495f3

This reverts commit 7987e6a, reversing changes made to d88d479.

RReverser mentioned this pull request Mar 19, 2014

Fixes license header and adds stable sort #1267

Closed

RReverser deleted the pure-cjs branch March 19, 2014 22:29

RReverser mentioned this pull request Aug 4, 2014

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

Closed

This was referenced Sep 16, 2021

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#86

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#87

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#88

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#89

Open

This was referenced Nov 5, 2022

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#161

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#162

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#163

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#164

Open

This was referenced Dec 25, 2022

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#187

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#188

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#190

Open

[Snyk] Fix for 1 vulnerabilities ryan-ally/react-1#191

Open

This was referenced Nov 27, 2023

[Snyk] Fix for 5 vulnerabilities ryan-ally/react-1#250

Open

[Snyk] Fix for 3 vulnerabilities ryan-ally/react-1#252

Open

[Snyk] Fix for 3 vulnerabilities ryan-ally/react-1#253

Open

This was referenced May 13, 2024

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#285

Open

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#286

Open

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#288

Open

[Snyk] Fix for 2 vulnerabilities ryan-ally/react-1#291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switched from browserify to pure-cjs bundler. #1234

Switched from browserify to pure-cjs bundler. #1234

RReverser commented Mar 8, 2014

zpao commented Mar 10, 2014

RReverser commented Mar 11, 2014

benjamn Mar 17, 2014

RReverser Mar 17, 2014

RReverser Mar 17, 2014

zpao commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

Switched from browserify to pure-cjs bundler. #1234

Switched from browserify to pure-cjs bundler. #1234

Conversation

RReverser commented Mar 8, 2014

zpao commented Mar 10, 2014

RReverser commented Mar 11, 2014

benjamn Mar 17, 2014

Choose a reason for hiding this comment

RReverser Mar 17, 2014

Choose a reason for hiding this comment

RReverser Mar 17, 2014

Choose a reason for hiding this comment

zpao commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014

zpao commented Mar 18, 2014

RReverser commented Mar 18, 2014