Skip to content
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

feat: Minify CSS/JS in production bundle #9095

Closed
wants to merge 2 commits into from

Conversation

kevcodez
Copy link
Contributor

@kevcodez kevcodez commented Nov 4, 2020

Addresses #9092

  • Minifies CSS/JS in production bundles

Checking the desktop GUI dist:

  • app.js 2356 kb => 921 kb (~ 60% reduction)
  • app.css 242 kb => 192 kb (~ 20% reduction)

User facing changelog

  • CSS/JS are now being minified to decrease overall bundle size

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Nov 4, 2020

Thanks for taking the time to open a PR!

@kevcodez kevcodez marked this pull request as ready for review November 4, 2020 18:07
@jennifer-shehane
Copy link
Member

We appear to be throwing an error ourselves, that we don't want the code minified.
Screen Shot 2020-11-05 at 12 38 05 PM

In our https://github.com/cypress-io/cypress/blob/develop/scripts/binary/util/testStaticAssets.js#L62:L62

I feel like this is meant to throw when we're doing a development build? I'm not sure it should be necessary to not minify the code for a production build.

It looks like @bkucera originally wrote a lot of this, so maybe he has some insight into your changes. #4103

@kevcodez
Copy link
Contributor Author

kevcodez commented Nov 5, 2020

Development builds should be unminified and production builds should be minified with source maps. That way development and production are debuggable. That's atleast the default for any web app - might be handled different for Electron apps - let me know.

@Andarist
Copy link
Contributor

Andarist commented Nov 7, 2020

Does this affect cypress_runner.js? If yes I would argue that this might not be ideal and might deteriorate developer experience quite heavily.

Being able to debug things is crucial for developers and this includes occasional changes to the source code. If I encounter a weird bug that I need to track down I would much more prefer to have a full suite of developer techniques at my disposal. If you minify the source code and only provide source maps then this is no longer true - to really be able to dive deep into things I would have to recompile source files without modification which raises the barrier level quite a bit.

@kevcodez
Copy link
Contributor Author

kevcodez commented Nov 7, 2020

@Andarist I see your point and agree. I've included this in the shared webpack config. So anything that uses the shared webpack config would be affected. Alternatively, I could also move it to specific applications like the desktop GUI. If you see the same issues there, you may close this PR.

@Andarist
Copy link
Contributor

Andarist commented Nov 7, 2020

I'm just an outside commenter here so this is definitely not my call. I'm just raising my concerns 😉

This shouldn't be as much of an issue with the desktop GUI - although I could see maybe somebody seeing it that way as well. This could be compared to a CLI tool - I usually don't need to debug it directly, but I've done it on multiple occasions. Depending on what exact kind of logic is put there, there might always be a need to debug smth.

@jennifer-shehane
Copy link
Member

Yes, it seems that @Andarist's concerns were our original reasoning behind not minifying the code. We want the experience of debugging issues to be a better experience in production.

I can also see this being less of a concern for the Desktop-GUI code, but I can bring it up with the team when I have a chance.

@jennifer-shehane
Copy link
Member

Closing as our team believes the amount of space saved (being under 1mb) is not worth the loss of user experience when debugging the code in production later.

@kevcodez kevcodez deleted the issue-9092 branch November 19, 2020 09:41
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.

None yet

3 participants