Skip to content

Example of regenerated minified files #765

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

Closed
wants to merge 1 commit into from

Conversation

myitcv
Copy link
Member

@myitcv myitcv commented Feb 27, 2018

DO NOT MERGE

Example of auto-regenerated prelude files; potential follow-up to #761

@myitcv myitcv force-pushed the regen_min branch 3 times, most recently from afc154c to c56bd43 Compare February 27, 2018 09:29
@dmitshur
Copy link
Member

Thanks for prototyping this @myitcv. This approach looks pretty much as I expected. 👍 I like the idea of using npm install to get the uglify-es dependency.

I still prefer not to automate this just yet, because the prelude doesn't change often and it's not much harder to do it as done in #761.

Should that change, we can reopen this PR and resume here. Thanks.

@dmitshur dmitshur closed this Feb 27, 2018
dmitshur added a commit that referenced this pull request Feb 27, 2018
Use uglify-es 3.3.9 via https://skalman.github.io/UglifyJS-online/
with default options.

Currently, it needs to be manually updated whenever prelude changes.
It's not very often. When that changes, it can be automated via #765.

Resolves #762.
Closes #761.

Co-authored-by: Dmitri Shuralyov <dmitri@shuralyov.com>
@myitcv
Copy link
Member Author

myitcv commented Mar 10, 2018

@shurcooL

I still prefer not to automate this just yet, because the prelude doesn't change often and it's not much harder to do it as done in #761.

As I understand things going the extra step to automate this (especially given the prototype confirms it's straightforward) is worthwhile because:

  • the alternative requires multiple manual steps (for each part of the prelude that changes)
  • it's impossible to tell if things are out of sync without running all of the manual steps
  • requires someone to remember that the manual step needs to be re-run... which requires someone to remember to check this during review time
  • the linked tool may change (far less likely with pinned npm deps)

Unless I'm missing something in the way the alternative works?

@dmitshur
Copy link
Member

dmitshur commented Mar 14, 2018

I think it's worthwhile if the prelude changes often. Historically, it has changed very little (a few times in the last few years), although the frequency might be increasing recently.

I think it's fair to wait until it changes 2-3 more times before going through with automating it. To me, that's easier.

@myitcv
Copy link
Member Author

myitcv commented Mar 17, 2018

I think it's fair to wait until it changes 2-3 more times before going through with automating it. To me, that's easier.

I'm not entirely sure I follow, particularly given the experience of #775 (specifically #775 (comment))

Because the work to automate it is already done; that's what this PR represents.

Or am I missing something?

Can I suggest I rebase this PR, ensure CI works and then we see what the diff looks like?

hajimehoshi pushed a commit that referenced this pull request Apr 20, 2018
This is a rebase of #765.

Whilst rebasing #669 I fell into a similar trap to #775 (#775 (comment)),
by forgetting to commit the manually minified prelude.

Then going through the manual process of manually minifying the prelude I wasted a whole load of
time installing goexec, missing something the first time etc, issues that this PR addresses by
automating the process, as previously discussed (#765 (comment)).
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