Skip to content

Conversation

vinicius73
Copy link
Contributor

Context

Although We are bundling each component speared file, every file is bundled with every dependency individually.

As examples

  • NcButton is bundled in 21 different files.
  • NcActions is bundled in 11 different files.

Using the code below, ate least, we will have 2 NcButton registered in different moments.

import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
import NcActions from '@nextcloud/vue/dist/Components/NcActions.js'

We already are able to use a different approach.

import { NcButton, NcActions } from '@nextcloud/vue'

It solves the original issue, now we will have only one registered NcButton

Solution

src/index.js

Remove import * as NcComponents from './components/index.js' and install statements.

It removes possible side effects and open the way for Tree Shaking1.

src/install.js

Create an install entry, to keep previous behaviors.

webpack.config.js

Enable module library target.
https://webpack.js.org/configuration/output/#outputlibrarytype

It improves the support for ESM builds. It will generate index.module.js.

Break Changes

Emoji functions

As all functions are directly expose, the function addRecent from src/functions/emoji have chanced to make clear what kind of function it is.

Now addRecent is called emojiAddRecent


This is related to #2959.


Footnotes

  1. webpack, Roolup and parcel

@vinicius73 vinicius73 added enhancement New feature or request 3. to review Waiting for reviews feature: emoji picker Related to the emoji picker component labels Sep 21, 2022
@vinicius73 vinicius73 force-pushed the feature/enable-treeshaking branch from b9f1100 to 4086036 Compare September 21, 2022 23:30
@vinicius73
Copy link
Contributor Author

I do not have any ideia why Cypress tests is not passing anymore...

Vinicius Reis added 5 commits September 22, 2022 09:29
@vinicius73 vinicius73 force-pushed the feature/enable-treeshaking branch from 4086036 to 82aeb06 Compare September 22, 2022 12:31
Vinicius Reis added 3 commits September 22, 2022 09:34
Signed-off-by: Vinicius Reis <[email protected]>
Signed-off-by: Vinicius Reis <[email protected]>
Signed-off-by: Vinicius Reis <[email protected]>
@vinicius73 vinicius73 force-pushed the feature/enable-treeshaking branch from 82aeb06 to 3ff23c7 Compare September 22, 2022 12:39
Copy link
Contributor

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Generally very nice, just a few comments/questions inline :)

@juliusknorr
Copy link
Contributor

Tested with deck and main bundle size went down in dev build from 9.9 to 7.3 MB 🥳

Signed-off-by: Vinicius Reis <[email protected]>
"cypress:update-snapshots": "cypress run-ct --env type=base --config screenshotsFolder=cypress/snapshots/base"
},
"main": "dist/ncvuecomponents.js",
"module": "dist/index.module.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an official naming convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is just to keep webpack config simple

@vinicius73 vinicius73 merged commit f9432e7 into master Sep 23, 2022
@vinicius73 vinicius73 deleted the feature/enable-treeshaking branch September 23, 2022 12:28
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Sep 23, 2022

@vinicius73 For Tasks the bundle size slightly increased (from 4.5 MB to 4.6 MB) when switching to global imports in nextcloud/tasks#2117. Do I do anything wrong? I never used the global import in Tasks before, though. Only ever imported via @nextcloud/vue/dist/.... Maybe then no change in bundle size is expected?

@vinicius73
Copy link
Contributor Author

Yes @raimund-schluessler it can happen, specially if you bundle your app before we ship a new version with this PR change.

There are 3 main possible reasons

  • The previous versions aren't esm compatible, so tree-shaking is impossible.
  • The support of webpack to generate ESM bundle/library is not good yet, so the esm bundle is not so good yet.
  • We can't use "sideEffect": false" in package.json (I don't know the reason, but cypress broke when I changed it) so the bundling process can't do the tree shaking in the better way.

So, will be some cases as the bundle will increase until we fully migrate to another solution like vite, and solve those problems.

@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Sep 23, 2022

Thanks for the info. I used npm link, so it should be tree-shakeable already. I guess it's one of the other reasons then.
I will then probably stay with the imports via dist and check back once in a while whether it improved 🙂

@vinicius73 vinicius73 mentioned this pull request Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement New feature or request feature: emoji picker Related to the emoji picker component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants