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

Move fomantic and jQuery to main webpack bundle #11997

Merged
merged 2 commits into from
Jun 28, 2020

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jun 20, 2020

This saves around 3 MB binary size by not including useless fomantic files in the build. Also, this allows us to move jQuery into the main bundle as well which eliminates a few HTTP requests.

Also included are webpack config changes:

  • split less and css loaders to speed up compliation
  • enable css sourcemaps
  • switch css minfier plugin to cssnano-webpack-plugin which works better for sourcemaps than the previous plugin

@silverwind
Copy link
Member Author

Overall, this seems to actually decrease webpack build time because of the split up of the CSS loaders. BTW, to get the binary size reduction, one has to manually delete public/fomantic.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 20, 2020
@lafriks lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jun 20, 2020
@lafriks lafriks added this to the 1.13.0 milestone Jun 20, 2020
@lunny
Copy link
Member

lunny commented Jun 21, 2020

But the index.js and index.css will be bigger than before? It will spend more time on user first visit.

@silverwind
Copy link
Member Author

silverwind commented Jun 21, 2020

The index chunk will be bigger but the downloaded JS/CSS before page is ready is the same because we depend on jquery/fomantic in a render-blocking way. Overall size might even be a bit smaller because polyfills are added to each chunk (around 30kB of JS per chunk) and we get less of that with less chunks. Essentially,

<script src="index.js"></script>

is the same or faster than the previous:

<script src="jquery.js"></script>
<script src="semantic.min.js"></script>
<script src="index.js"></script>

@silverwind
Copy link
Member Author

Another nice benefit here is that we avoid unnecessary CSS reflows when the stylesheets load in because they now apply all at once.

@lunny
Copy link
Member

lunny commented Jun 22, 2020

CI failed

@silverwind
Copy link
Member Author

Failure at TestToUTF8WithFallback is unrelated.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jun 23, 2020
Makefile Outdated Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

The CSS source-map issue seems to persist even with these changes, our Webpack config seems to refuse to create them

@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

For what it's worth, we should most likely also provide sourcemaps for other resources for easier debugging stuff like Monaco or Service Worker.

@silverwind
Copy link
Member Author

silverwind commented Jun 27, 2020

Maybe it's because only js/index.js is whitelisted for sourcemap generation.

So far I've only had it enabled for the index chunk because source maps do blow up binary size considerably and for CSS, I'm not sure of their benefit but I guess it now gets more imporant with fomantic bundled in.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

I can't get it working even with explicit SourceMapDevToolPlugin removed and devtool set to source-map which normally generates all sourcemaps for everything else.

@silverwind
Copy link
Member Author

For what it's worth, we should most likely also provide sourcemaps for other resources for easier debugging stuff like Monaco or Service Worker.

Serviceworker I agree but huge dependencies Like Monaco, I don't. Those would be like 20mb source map with questionable benefit.

@silverwind
Copy link
Member Author

index.css source map added in db7f857, give it a try.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

Serviceworker I agree but huge dependencies Like Monaco, I don't. Those would be like 20mb source map with questionable benefit.

Do it for development only? Certainly it's huge but without it debugging issues with Monaco might be hard.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

I don't see sourcemap generated for CSS nor does Firefox or Chromium load one.

@silverwind
Copy link
Member Author

silverwind commented Jun 27, 2020

It works for me:

             css/index.css  1000 KiB            index  [emitted]        index
         css/index.css.map  1.33 MiB            index  [emitted] [dev]  index

In Chrome I see some filenames:

image

Certainly it's huge but without it debugging issues with Monaco might be hard

I guess generally we don't need to debug our dependencies and I'm not eager to slow down development build time even more (it's already quite slow, currently 21s for me, goes to 25s with all maps enabled).

@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

Are you sure you don't have any local changes? It refuses to create map for CSS files for me no matter what.

webpack.config.js Outdated Show resolved Hide resolved
@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

Weird, It seems to create it on watch-frontend but not on frontend?

@silverwind
Copy link
Member Author

Yes, just checked in a fresh clone. Try make clean-all && rm -rf node_modules.

@silverwind
Copy link
Member Author

Ah, apparently I get a CSS sourcemap on watch-frontend but not on frontend, strange.

@CirnoT
Copy link
Contributor

CirnoT commented Jun 27, 2020

Yes, just checked in a fresh clone. Try make clean-all && rm -rf node_modules.

Done and still happens only on watch-frontend

@silverwind
Copy link
Member Author

Apparently it's NMFR/optimize-css-assets-webpack-plugin#53 (comment). When I set this, it outputs source maps for all CSS files and completely seems to disregard the SourceMapDevToolPlugin's include option.

BTW, I removed the serviceworker map again, it's like 10 lines of our own source code and 99% workbox code and I don't see a reason to reserve another username for that.

@silverwind
Copy link
Member Author

silverwind commented Jun 27, 2020

Fixed CSS sourcemaps by using cssnano-webpack-plugin which looks to better integrate with SourceMapDevToolPlugin.

Also included is a fix that prevents unnecessary fomantic rebuilds when package-lock.json changes. Instead, it now only rebuilds when semantic.json changes (which we will always update when updating fomantic).

@silverwind silverwind changed the title Move fomantic and jQuery to main bundle Move fomantic and jQuery to main webpack bundle Jun 27, 2020
@silverwind silverwind force-pushed the fomantic-webpack branch 2 times, most recently from 0285bad to 269091c Compare June 27, 2020 18:44
@silverwind
Copy link
Member Author

Squashed and update commit title and message.

This saves around 3 MB binary size by not including useless fomantic
files in the build. Also, this allows us to move jQuery into the main
bundle as well which eliminates a few HTTP requests.

Also included are webpack config changes:

- split less and css loaders to speed up compliation
- enable css sourcemaps
- switch css minfier plugin to cssnano-webpack-plugin which works better
  for sourcemaps than the previous plugin
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 27, 2020
@techknowlogick
Copy link
Member

ping LG-TM

@techknowlogick techknowlogick merged commit 7761245 into go-gitea:master Jun 28, 2020
@silverwind silverwind deleted the fomantic-webpack branch June 28, 2020 04:22
silverwind added a commit to silverwind/gitea that referenced this pull request Jul 1, 2020
The filter was wrongly excluding the gitGraph.css file. Need to clean
this up later so that imports are always relative to the source file
(which is not the case for fonts right now).

Regressed by: go-gitea#11997
zeripath added a commit that referenced this pull request Jul 1, 2020
The filter was wrongly excluding the gitGraph.css file. Need to clean
this up later so that imports are always relative to the source file
(which is not the case for fonts right now).

Regressed by: #11997

Co-authored-by: zeripath <art27@cantab.net>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
This saves around 3 MB binary size by not including useless fomantic
files in the build. Also, this allows us to move jQuery into the main
bundle as well which eliminates a few HTTP requests.

Also included are webpack config changes:

- split less and css loaders to speed up compliation
- enable css sourcemaps
- switch css minfier plugin to cssnano-webpack-plugin which works better
  for sourcemaps than the previous plugin

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
The filter was wrongly excluding the gitGraph.css file. Need to clean
this up later so that imports are always relative to the source file
(which is not the case for fonts right now).

Regressed by: go-gitea#11997

Co-authored-by: zeripath <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants