Reduce bundle size, add code splitting #1116

MarcelRobitaille · 2022-07-26T05:00:22Z

I'm writing this as a PR only so you can see the code changes. I don't intend this to be merged as is. I am just trying to continue the code splitting discussion.

Fixes #455
Fixes #1109

I implemented "naive" code splitting. I say naive because I just did every different route. However, these routes are usually quite small. We might be better off including the truly tiny ones in the main bundle to minimize the number of network requests.

Here is the bundle analyzer for the master branch.

cookbook-main.js is 4.79 MB raw, 1.05 MB gzipped.
cookbook-guest.js is 1.06 MB raw, 277 kB gzipped.

This small change seems to have inadvertently split some node modules into their own chunks. Now we have:

cookbook-main.js 3.67 MB raw, 872 kB gzipped
cookbook-guest.js 1.06 MB raw, 277 kB gzipped.
moment 711 kB raw, 112.97 kB gzipped
RecipeView.js 87 kB raw, 16 kB gzipped
Multiselect.js 173 kB raw, 45 kB gzipped
RecipeList.js 63 kB raw, 13 kB gzipped
RecipeEdit.js 114 kB raw, 21 kb gzipped
NotFound.js 4 kB raw, 1.9 kB gzipped
SearchResult.js 4.7 kB raw, 1.9 kB gzipped
AppIndex.js 3 kB raw, 1.5 kB gzipped

Apparently, the chunks will be prefetched in Vue 3.0. I was not able to make them prefetch with the webpack magic comment import(/* webpackPrefetch: true */ '') for some reason. I am also unsure how to change the long file names. I tried output: { filename: '' } in the webpack config and the magic comment /* webpackChunkName: "" */, but no change.

I got some good savings by dynamically loading the moment locale:

cookbook-main.js is almost unchanged and only one locale needs to be loaded

I think we may be better off going after npm packages than our own code.Maybe we can lazy load the mardown editor, replace ActionInput, etc. I'm not sure why calendar-js and ical are needed, but both are pretty big. They seem to not be imported anywhere. I only see reference to them in package-lock.json as a dependency of nextcloud/vue.

Maybe we can also look into tree shaking.

github-actions · 2022-07-26T05:05:38Z

Unit Test Results

    27 files     27 suites 6m 57s ⏱️
  476 tests   476 ✔️ 0 💤 0 ❌
4 284 runs 4 283 ✔️ 1 💤 0 ❌

Results for commit 3bf88f5.

♻️ This comment has been updated with latest results.

christianlupus · 2022-07-26T08:49:30Z

Apparently, the chunks will be prefetched in Vue 3.0. I was not able to make them prefetch with the webpack magic comment import(/* webpackPrefetch: true */ '') for some reason. I am also unsure how to change the long file names. I tried output: { filename: '' } in the webpack config and the magic comment /* webpackChunkName: "" */, but no change.

We are not on Vue 3 but Vue 2. Thus the prefetch might not (yet) be implemented.
Why do you want to change the file names? They are configured in @nextcloud/webpack-vue-config

Maybe we can also look into tree shaking.

I tree shaking not done/present ATM? Moving other (sub-) components (like the Markdown editor/parser) to their own chunk might be a good option.

MarcelRobitaille · 2022-07-26T19:38:02Z

We are not on Vue 3 but Vue 2. Thus the prefetch might not (yet) be implemented.

I know. It's supposed to be automatic in Vue 3. As far as I understand, you're supposed to be able to manually enable it for any framework with those webpack magic comments. It's a webpack feature, so it should be independent of the Vue version. I am not sure why it's not working.

Why do you want to change the file names? They are configured in @nextcloud/webpack-vue-config

The auto-generated chunk names are very long, like cookbook-vendors-node_modules_nextcloud_vue_dist_Components_Multiselect_js.js. I would prefer to have cookbook-appindex.js over cookbook-src_components_AppIndex.js for example. I guess it's done to guarantee a unique name, but we know we only have one AppIndex.

I will try to move out the markdown editor.

MarcelRobitaille · 2022-07-26T20:51:56Z

I moved Showdown to RecipeView because that's the only place it's used.

I also implemented my own ActionInput. All I did was remove the bloated imports like DatetimePicker which imports @nextcloud/calendar-js and ical, which are both very big. However, now we're getting into a problem of copying GPL stuff. Maybe we can make a PR to dynamically load DatetimePicker. Unfortunately, this is all packaged into @nextcloud/vue/dist/Components/ActionInput.js, so there is no option to block these imports with webpack IgnorePlugin. We could get around this by depending on the source code version of @nextcloud/vue rather than pre-compiled. Then, the IgnorePlugin would work and we wouldn't need to modify GPL code.

cookbook-main.js is down over 2 MB from starting.

Edit: I just rebased after the merge of #1105 and we are down another 0.2 MB.

Raw:

Gzipped:

MarcelRobitaille · 2022-10-18T15:12:55Z

I think this PR is ready for review.

Unfortunately, the way in which the bloat has to be stripped from some libraries requires hacks like this. This makes it possible for one of these changes to cause unexpected breakage. I tried to test as best as I could, but I would appreciate if somebody else could take a look too to double check. I have manually tested:

ActionInput in AppControls
ActionInput in AppNavi
Markdown rendering in RecipeView: Showdown was moved out of main and only into RecipeView where it is used
moment: I changed the language to French and checked that the locale was dynamically loaded and that moment.locales() included fr
Going to all pages to check that loading the components dynamically is working properly

Also, I copied and modified @nextcloud/vue's ActionInput.vue. I gave the attribution, and we're both AGPL 3.0, so it should be fine, right? I can also try to contribute this upstream, but I'm not sure if it would get merged.

christianlupus · 2022-10-20T18:14:57Z

I think this PR is ready for review.

Man, you ran me over with that one.

Unfortunately, the way in which the bloat has to be stripped from some libraries requires hacks like this. This makes it possible for one of these changes to cause unexpected breakage. I tried to test as best as I could, but I would appreciate if somebody else could take a look too to double check.

I feel a bit overwhelmed and also not qualified enough to oversee all things here. Looks like magic to me currently. (Maybe a bit more sleep might help 😉.) Would you @seyfeb consider having a look at this once? I can click around a bit and see if there is some problem with anything I can see. However, this will be only of limited outcome.

Also, I copied and modified @nextcloud/vue's ActionInput.vue. I gave the attribution, and we're both AGPL 3.0, so it should be fine, right? I can also try to contribute this upstream, but I'm not sure if it would get merged.

The license should be ok if the upstream is GPL. I would find it very important if this were at least offered upstream.

This thing with the source version sounded to me like the best solution but I am completely lost how this would be achieved with npm.

seyfeb · 2022-10-21T08:42:56Z

I feel a bit overwhelmed and also not qualified enough to oversee all things here. Looks like magic to me currently. (Maybe a bit more sleep might help 😉.) Would you @seyfeb consider having a look at this once? I can click around a bit and see if there is some problem with anything I can see. However, this will be only of limited outcome.

I can try to have a look!

MarcelRobitaille · 2022-10-21T14:26:09Z

Maybe I can summarise the changes:

Code splitting: Each view has its own smaller bundle now. When going to a page, only the common bundle and that page's bundle are loaded. This reduces the main bundle greatly. The relevant change is here.
Only load VueShowdown in RecipeView. The library is only used on that page, so it can go in that page's bundle instead of the main bundle. The relevant change is here and here.
Only load the required moment locale. First, the moment locales are stripped from the main bundle. There is no "official" way to do this, so we have to do it in a kind of hacky way. Then, because the main bundle doesn't have any locales, we have to dynamically load the user's locale. This is done here.
Finally, the ActionInput component from @nextcloud/vue has many features that we do not use. These features have large dependencies like a calendar dropdown. The dependencies are included in the bundle even if those features aren't used. Therefore, I forked that component and stripped out the dependencies and features that we don't use. I called this SimpleActionInput. This is used here and here.

seyfeb · 2022-10-25T16:05:21Z

Not sure if this was introduced with this PR and/or if it's an artifact of my local dev machine but when starting from a clean installation (no recipes) and reload the cookbook page I get an error logged (not always the same, but only one per reload). The issue seems to occur while loading the chunks with webpackPrefetch annotation!?

ChunkLoadError: Loading chunk vendors-node_modules_nextcloud_vue_dist_Components_Multiselect_js failed.

``` ChunkLoadError: Loading chunk vendors-node_modules_nextcloud_vue_dist_Components_Multiselect_js failed. (error: http://localhost:8000/apps/cookbook/js/cookbook-vendors-node…vue_dist_Components_Multiselect_js.js?v=e91c46ff305fc2da743a) j jsonp chunk loading:27 e ensure chunk:6 e ensure chunk:5 component index.js:11 Oe vue-router.esm.js:2144 ze vue-router.esm.js:2171 ze vue-router.esm.js:2171 ze vue-router.esm.js:2170 Oe vue-router.esm.js:2106 g vue-router.esm.js:2362 o vue-router.esm.js:2087 o vue-router.esm.js:2091 De vue-router.esm.js:2095 confirmTransition vue-router.esm.js:2392 transitionTo vue-router.esm.js:2260 init vue-router.esm.js:2996 beforeCreate vue-router.esm.js:1298 VueJS 4 main.js:66 main.js:72 main.js:72 ```

ChunkLoadError: Loading chunk src_components_AppIndex_vue failed.

``` ChunkLoadError: Loading chunk src_components_AppIndex_vue failed. (error: http://localhost:8000/apps/cookbook/js/cookbook-src_components_AppIndex_vue.js?v=b8e1c17bc2a8a1c22654) j jsonp chunk loading:27 e ensure chunk:6 e ensure chunk:5 component index.js:11 Oe vue-router.esm.js:2144 ze vue-router.esm.js:2171 ze vue-router.esm.js:2171 ze vue-router.esm.js:2170 Oe vue-router.esm.js:2106 g vue-router.esm.js:2362 o vue-router.esm.js:2087 o vue-router.esm.js:2091 De vue-router.esm.js:2095 confirmTransition vue-router.esm.js:2392 transitionTo vue-router.esm.js:2260 init vue-router.esm.js:2996 beforeCreate vue-router.esm.js:1298 VueJS 4 main.js:66 main.js:72 main.js:72 ```

ChunkLoadError: Loading chunk src_components_RecipeList_vue failed.

``` ChunkLoadError: Loading chunk src_components_RecipeList_vue failed. (error: http://localhost:8000/apps/cookbook/js/cookbook-src_components_RecipeList_vue.js?v=66d13bba91cddcc8c16f) j jsonp chunk loading:27 e ensure chunk:6 e ensure chunk:5 component index.js:11 Oe vue-router.esm.js:2144 ze vue-router.esm.js:2171 ze vue-router.esm.js:2171 ze vue-router.esm.js:2170 Oe vue-router.esm.js:2106 g vue-router.esm.js:2362 o vue-router.esm.js:2087 o vue-router.esm.js:2091 De vue-router.esm.js:2095 confirmTransition vue-router.esm.js:2392 transitionTo vue-router.esm.js:2260 init vue-router.esm.js:2996 beforeCreate vue-router.esm.js:1298 VueJS 4 main.js:66 main.js:72 main.js:72 vue-router.esm.js:2316:16 l vue-router.esm.js:2316 g vue-router.esm.js:2369 u vue-router.esm.js:2138 Fe vue-router.esm.js:2203 (Async: promise callback) Oe vue-router.esm.js:2150 ze vue-router.esm.js:2171 ze vue-router.esm.js:2171 ze vue-router.esm.js:2170 Oe vue-router.esm.js:2106 g vue-router.esm.js:2362 o vue-router.esm.js:2087 o vue-router.esm.js:2091 De vue-router.esm.js:2095 confirmTransition vue-router.esm.js:2392 transitionTo vue-router.esm.js:2260 init vue-router.esm.js:2996 beforeCreate vue-router.esm.js:1298 VueJS 4 main.js:66 main.js:72 main.js:72 ```

christianlupus · 2022-10-25T19:28:09Z

This could be due to the fact that you are running (like me) from the /custom_apps/cookbook location. Please have a look in the network tab of the dev console for failing java script chunks. Are they with the prefix /apps or /custom_apps? The chunks should be present after billing with npm in the js folder. If yes, the correct url is with the real filesystem path (aka /custom_apps/cookbook).

seyfeb · 2022-10-25T20:02:43Z

Your right, the path is wrong, didn't notice the apps vs custom_apps directory issue. I saw the files in the js/ folder and was wondering why they couldn't be loaded. How did you setup the build to use the correct custom_apps path, @christianlupus ?

christianlupus · 2022-10-26T06:46:35Z

I did not test yet. I think, we might need to add something like

__webpack_public_path__ = linkTo('cookbook', 'js')

in the main.js. Webpack computes the paths to be /apps/cookbook which is valid if the app is installed in apps. Otherwiese we need to modify things using the public path.

This was the reason for 48188dc.

seyfeb

I did not go through each line, but in general it looks good to me.

One thing that bothers me, is the SimpleActionInput. I (hopefully) does the same thing as Nextcloud's ActionInput, but cannot do more than the way it is currently used. Using more functionality would require reimplementing or copying stuff over from the original component. Drawbacks are: It is a very crowded file, because lots of previously imported stuff (styles and JS) were copied into the single file; it needs to be maintained by ourselves instead of relying on a separately maintained component.

Afaik, the main reason to use this, was to reduce the bundle size. How much does the change to using the component actually save?

src/components/SimpleActionInput.vue

src/main.js

MarcelRobitaille · 2022-10-26T09:55:08Z

Thanks for reviewing it @seyfeb

Afaik, the main reason to use this, was to reduce the bundle size. How much does the change to using the component actually save?

Correct. The savings is about 750kB.

With @nextcloud/vue ActionInput the main.js is 2.25MB
With my SimpleActionInput the main is 1.49MB

The reason is that @nextcloud/vue is using multiple large libraries to facilitate the calendar features. I have highlighted some of the big ones in the below bundle analyzer screenshot (I may have missed some of the smaller ones).

One thing that bothers me, is the SimpleActionInput. I (hopefully) does the same thing as Nextcloud's ActionInput, but cannot do more than the way it is currently used. Using more functionality would require reimplementing or copying stuff over from the original component. Drawbacks are: It is a very crowded file, because lots of previously imported stuff (styles and JS) were copied into the single file; it needs to be maintained by ourselves instead of relying on a separately maintained component.

I agree. I don't love this solution either. In my opinion, 33% savings is worth it.

One easy thing that I think actually has a chance of being upstreamed is to use dynamic import()s for those libraries only if those features are used. This would have a similar effect for us while not removing functionality. I think it would be a pretty small code change too.

github-actions · 2022-10-26T10:23:36Z

Test Results

    21 files ±0   952 suites ±0 4m 25s ⏱️ -4s
  495 tests ±0   495 ✔️ ±0 0 💤 ±0 0 ❌ ±0
3 465 runs ±0 3 464 ✔️ ±0 1 💤 ±0 0 ❌ ±0

Results for commit 3febd8f. ± Comparison against base commit 1a97322.

♻️ This comment has been updated with latest results.

seyfeb · 2022-10-26T11:52:07Z

Okay that's actually a lot more than suspected! This might indeed be worth the trouble (you already did the most work anyway). If you see dynamic imports as a better solution, you may change this, but either way I'm fine with the PR

MarcelRobitaille · 2022-10-26T12:16:52Z

Would you prefer the dynamic import version for our own use or would it only be to upstream it and remove our own custom thing? I think we should keep SimpleActionInput as it is now, at least until we actually use the other features. I'll try to get the dynamic import version working and then make a PR upstream.

seyfeb · 2022-10-26T15:43:30Z

I'm okay with that :)

christianlupus · 2022-10-27T11:41:13Z

What did you do, @seyfeb, to get it running? This patch was not yet included, am I wrong?

seyfeb · 2022-10-27T12:09:25Z

What did you do, @seyfeb, to get it running? This patch was not yet included, am I wrong?

Naah, because I hacked it in there to get it running on my local machine. I did override the config of the publicPath in webpack.dev.js by adding

    output: {
        publicPath: path.join('/custom_apps/', appName, '/js/'),
    },

christianlupus · 2022-10-27T12:10:37Z

What did you do, @seyfeb, to get it running? This patch was not yet included, am I wrong?

Naah, because I hacked it in there to get it running on my local machine. I did override the config of the publicPath in webpack.dev.js by adding
    output: {
        publicPath: path.join('/custom_apps/', appName, '/js/'),
    },

This is not good. It will break any other installation without the /custom_apps prefix out there. We need to address this before merging.

christianlupus

Make the path where the parts are loaded generic and independent of the storage path of the app.

PS: This is just to prevent accidental merging

seyfeb · 2022-10-27T12:17:18Z

It's only an issue if you manually add it to custom_apps though? apps installed to apps/ (via the app store) should just work, no?

christianlupus · 2022-10-27T14:02:27Z

Unfortunately no. In the official admin documentation of the server explicitly allows to have different storage locations for apps. I guess that many have the default prefix of apps but this is just a guess. For sure, this will not be working well in the other cases.

seyfeb · 2022-10-27T14:49:30Z

I see. I'm a bit confused, but what seems to be working is setting

webpackConfig.output.publicPath = undefined

in webpack.config.js, hence overriding the definition of the nextcloud default config, which confusingly sets it to publicPath: path.join('/apps/', appName, '/js/')

I'm not sure if this makes sense.

christianlupus · 2022-10-27T15:43:32Z

I have used the __webpack_public_path__ approach to test it. For me, I get CSP errors:

Can you please check that your browser loads all imports as desired? I am using Firefox on Linux.

seyfeb · 2022-10-27T17:54:52Z

With my approach described above I'm seemingly on the good side of things. (Firefox on macOS) Although I just noiticed, the search is not working for some reason. It switches to the search page, and the data JSON data is received, but it seems not to be loaded into the component.

Another note: It, e.g., does not load SearchResults although its specified to be prefetched in index.js, but also does not error on loading. It simply doesn't appear in the Network tab. When actually executing a search, the component gets loaded without an error.