react icon indicating copy to clipboard operation
react copied to clipboard

Decide on whether to bundle React by default or not

Open myitcv opened this issue 8 years ago • 6 comments

Revised 2017-03-29 following resolution on https://github.com/gopherjs/gopherjs/issues/611

As of f51106b7ade5da288a9b798bb6c700d59032b0a0 whenever the github.com/myitcv/gopherjs/react package is included as part of a GopherJS build/serve, we bundle React's .js files by default. That is to say in effect React's .js files get loaded as the served/built GopherJS output is loaded.

For context, React's .js files come in two flavours:

  • development: non-minified, contains helpful warnings etc, larger and slower
  • production: minified, minimal, fastest

This issue is designed to track discussion the decision on whether to bundle or not by default. Specifically there are two options:

Option 1: bundle by default

gopherjs build                       =>  includes React production (minified) .js files
gopherjs build --tags noReactBundle  =>  do _not_ bundle ANY React .js files

Option 2: require explicit build tag to bundle

gopherjs build                        =>  React not bundled; must have been loaded separately
gopherjs build --tags bundleReact     =>  includes React production (minified) .js files

The use of the GopherJS -m minify flag is orthogonal to either of the above options (following the conclusion on https://github.com/gopherjs/gopherjs/issues/611)

As is the point being decided in #31 (for now we bundle the production version, with the development option available via the debug build tag)

myitcv avatar Mar 20 '17 19:03 myitcv

@edrex @dotMR and @evmar might have welcome opinions on that.

mpl avatar Mar 22 '17 14:03 mpl

At first glance, it would seem that Option 1 is simpler, because only 3 cases seem to arise from it, but that's just because you didn't include the following case:

gopherjs build -m --tags noReactBundle

In this case, the '-m' flag would have no effect, but that's essentially the same as the following Option 2 case, which you did include:

gopherjs build -m

Just from a logic point of view, I prefer option 2, because "affirmative action" tags/booleans are more readable (sometimes it can't be avoided, but setting a bool to true to disable something is awkward). From a client pov (Camlistore), I also prefer option 2, since we already have to embed react.js in our binaries.

mpl avatar Mar 28 '17 13:03 mpl

@mpl thanks for the shout-out, no real opinion on this - i'm pretty far removed at the moment. Besides, the whole go -> JS thing scares me :) With that said, your reasoning for Option 2 seem ok

dotMR avatar Mar 28 '17 13:03 dotMR

@mpl - thanks for the comments.

Apologies, I forgot to update the discussion in this PR when https://github.com/gopherjs/gopherjs/issues/611 was closed. I need to do that as it has changed the proposed default. I will update the description later and ping back.

myitcv avatar Mar 28 '17 13:03 myitcv

@dotMR booo! ;P Once you get past the "tiny" fact that the generated js is gigantic, gopherjs is wonderful. It feels so empowering to be able to fix/add to the UI just by writing Go. We still accept js contributions of course, in case you ever feel like it.

@myitcv sorry for the thread hijacking, I'm done.

mpl avatar Mar 28 '17 22:03 mpl

@mpl - updated the issue description, also clarifying the point about -m (which was only salient because of https://github.com/gopherjs/gopherjs/issues/611, which is now closed)

myitcv avatar Mar 29 '17 08:03 myitcv