-
Notifications
You must be signed in to change notification settings - Fork 570
Proposal: minify flag automatically adds minify build tag #611
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
Comments
I think I'm okay with doing this, as long as there are no good reasons why this shouldn't be done. So far, I can't think of why adding this would be bad... But still thinking. Once we add this, it'll be much harder to remove it. Is there anything similar in the standard Go toolchain? I know it automatically sets build tags for the current Go version, as well as architecture and OS. But is there anything else that it does that would be more akin to the minify flag/build tag? |
I'd personally rather see support for commonjs in node_modules. The biggest problem with *.inc.js is that you'll get tons of duplicate sub-dependencies for modules using underscore etc, which is unfortunately quite common. |
Very true, hence important we give it some thought.
I very much doubt it, because the Go tool chain works extremely hard to avoid compiler flags. With GopherJS it's more an artefact of the Javascript/web/frontend world that we have the concept of minification and hence the @tj - not quite sure I understand what you mean. Can you explain more? The concept of Rather in this case as the writer of a package of GopherJS bindings, I've made an explicit decision that I want to bundle some Javascript (via a package |
Sorry yea wrong place to discuss the inc.js stuff, ignore me :D |
I recall a discussion where possibly Rob Pike came down firmly against the suggestion of a build tag for testing mode. I might be able to find it when I am back from vacation in 3 weeks if necessary. But the justification was essentially "code should work the same in production as in testing." So while a "minify" tag might seem in principle the same as a "test" tag, I'm not sure if that reasoning applies. Personally, I have little opinion on this matter. I only wonder if it's worth committing ourselves to supporting this flag essentially forever, only to save a few characters in a few build scripts. |
That's actually very similar to the main concern that I've got so far. Introducing this creates the possibility of someone abusing the |
@flimzy @shurcooL - thanks. Really useful discussion
I recall having seen a number of discussions along these lines (hence the comment above about compiler flags). And I completely agree with the sentiment. A few thoughts. Minification should not, by definition, change the behaviour of Javascript code. At least I assume when we talk about minification in the GopherJS world we're talking about the So in that respect, the minify flag is not changing the behaviour of our code; to my mind it totally honours the letter and spirit of Rob's comments. The Facebook strategy of Facebook have two versions of the React
The development version is somewhat analogous in my mind to the race detector mode of the Go toolchain. Facebook happen to distribute the development version unminified (to help with debugging) and the production version pre-minified. But the decision to pre-minify the production version is irrelevant; I could just as easily make the decision that I want to minify the development version in my GopherJS app; again, minification doesn't change the behaviour. So I think the discussion boils down to these two separate points:
This proposal is about point 1; I'm minded given the discussion to think we probably don't have evidence that the flag set The discussion about point 2 is very valuable. I'll create a separate issue in |
Here is that issue: myitcv#31 |
👍 |
Great in which case I close this for now. Thanks again for the fruitful discussion! |
Headline proposal
When the
-m
(--minify
) flag is provided to those commands that can accept it, add theminify
build tag automatically.Detail
This proposal is principally aimed at the use of
.inc.js
files.I'm now bundling the React javascript files with my React bindings via a similar approach to that described by @flimzy
The choice of whether to import a package that includes a minified Javascript file or the full "dev" version is currently based on the
debug
build flag:I'd like to propose that when the
-m
(--minify
) is provided this automatically sets theminify
build tag (e.g.gopherjs serve -m
would be equivalent in effect togopherjs serve -m --tags minify
). The above approach could then switch to using the invertedminify
build flag and the net effect would be exactly as expected.This will allow bindings developers to unify on a common approach for use of
.inc.js
files (irrespective of whether #468 is resolved)Thoughts?
The text was updated successfully, but these errors were encountered: