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

chore: Make dependencies external #477

Merged
merged 1 commit into from
Feb 16, 2023
Merged

Conversation

juliusknorr
Copy link
Contributor

Before:

✓ 282 modules transformed.
dist/style.css 0.21 KiB / gzip: 0.15 KiB
dist/main.js 717.44 KiB / gzip: 203.73 KiB
dist/main.js.map 1937.99 KiB

After:

✓ 15 modules transformed.
dist/style.css 0.21 KiB / gzip: 0.15 KiB
dist/main.js 37.21 KiB / gzip: 7.25 KiB
dist/main.js.map 136.01 KiB

For nextcloud/server#36384

Copy link
Contributor

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎

import NcModal from '@nextcloud/vue/dist/Components/NcModal.js'
import NcNoteCard from '@nextcloud/vue/dist/Components/NcNoteCard.js'
import NcPasswordField from '@nextcloud/vue/dist/Components/NcPasswordField.js'
import { NcButton, NcModal, NcNoteCard, NcPasswordField } from '@nextcloud/vue'
Copy link
Contributor

@susnux susnux Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a good idea as long as we use webpack for applications!

Because you set the dependency to external¹ webpack now has to treeshake and it is quite bad doing it.
In numbers, a simple app just consisting of the example code in the readme:

With this: 9543055 Bytes ~ 9,1 MiB
When keeping the default import: 2674259 Bytes ~ 2,55 MiB

¹: The testapp output size, when keep changing the import but not setting @nextcloud/vue to external, is - thanks to better tree shaking of vite on this library - 4139035 Bytes ~ 4MiB.

EDIT: I just want to clarity that the size I wrote above is the whole js directory including the source map (du -bs js/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for that insight @susnux I think this is really bad for app that use more components and we should rather consider moving everything to importing the default export of the lib instead of the dist folder. As far as my tests went the dist imports are not tree-shakable at all.

There is probably no optimal way (except from moving away from web pack to site), but the index.module.js of @nextcloud/vue is currently at 883K which seems to be a value that a lot of apps and also server easily exceed with the individual modules from dist.

Just out of curiosity, du you have the small test app in some repo? I'd be curious to see the actual impact in the bundles with webpack-bundle-analyzer or vite-bundle-visualizer

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea to let vite do the tree-shaking here then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is really bad for app that use more components and we should rather consider moving everything to importing the default export of the lib instead of the dist folder. As far as my tests went the dist imports are not tree-shakable at all.

I tried this for the forms app (just js files):
dist import: 9.694 MiB (main branch)
default: 13.524 MiB (feat/move-to-module-import branch)

I think for dist import only the really required parts are included while importing from default export might include some parts which are not flagged side effects free, like the emoji mart is always included (about 1MiB), e.g. compare:

`dist` import:

Screenshot_20230208_124610

default import:

Screenshot_20230208_124503

Just out of curiosity, du you have the small test app in some repo? I'd be curious to see the actual impact in the bundles with webpack-bundle-analyzer or vite-bundle-visualizer

No but it is not hard to create, see below:

Simply create a new project, install @nextcloud/password-confirmation (use this branch) and @nextcloud/webpack-vue-config.
Create a file src/main.js with this content:

import { confirmPassword } from '@nextcloud/password-confirmation'
import '@nextcloud/password-confirmation/dist/style.css' // Required for dialog styles

export const foo = async () => {
    try {
        await confirmPassword()
        // Your logic
    } catch (error) {
        // Your error handling logic
    }
}

and webpack.js with

const path = require('path')
const webpackConfig = require('@nextcloud/webpack-vue-config')

module.exports = webpackConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the insights. I reverted the import changes for now and just added the packages as external.

Switching the imports would still be good for larger apps that include more components anyways, but lets's keep this separate for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will work better when the vue components are provided as a esm package. Then even webpack can treeshake it more efficient.

Switching the imports would still be good for larger apps

yes for all apps that e.g. include the emoji mart (one of the biggest dependencies), like the text app or the collectives app, this will be much better.

@juliusknorr juliusknorr merged commit 556042c into master Feb 16, 2023
@juliusknorr juliusknorr deleted the enh/package-extenral branch February 16, 2023 14:48
@susnux susnux added the bug Something isn't working label Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants