Skip to content
This repository has been archived by the owner on Aug 27, 2018. It is now read-only.

Decide on whether to bundle React by default or not #28

Open
myitcv opened this issue Mar 20, 2017 · 6 comments
Open

Decide on whether to bundle React by default or not #28

myitcv opened this issue Mar 20, 2017 · 6 comments
Labels

Comments

@myitcv
Copy link
Owner

myitcv commented Mar 20, 2017

Revised 2017-03-29 following resolution on gopherjs/gopherjs#611

As of f51106b 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 gopherjs/gopherjs#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 myitcv changed the title Provide build tag to opt out of ReactJS bundle? Decide on default for bundling React Mar 21, 2017
@myitcv myitcv changed the title Decide on default for bundling React [WIP] Decide on default for bundling React Mar 21, 2017
Repository owner locked and limited conversation to collaborators Mar 21, 2017
@myitcv myitcv changed the title [WIP] Decide on default for bundling React [WIP] Decide on whether to bundle React by default or not Mar 22, 2017
Repository owner unlocked this conversation Mar 22, 2017
@myitcv myitcv changed the title [WIP] Decide on whether to bundle React by default or not Decide on whether to bundle React by default or not Mar 22, 2017
@mpl
Copy link

mpl commented Mar 22, 2017

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

@mpl
Copy link

mpl commented Mar 28, 2017

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.

@dotMR
Copy link

dotMR commented Mar 28, 2017

@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

@myitcv
Copy link
Owner Author

myitcv commented Mar 28, 2017

@mpl - thanks for the comments.

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

@mpl
Copy link

mpl commented Mar 28, 2017

@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.

@myitcv
Copy link
Owner Author

myitcv commented Mar 29, 2017

@mpl - updated the issue description, also clarifying the point about -m (which was only salient because of gopherjs/gopherjs#611, which is now closed)

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

No branches or pull requests

3 participants