-
Notifications
You must be signed in to change notification settings - Fork 85
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
Enables ESM Modules #3283
Enables ESM Modules #3283
Conversation
b9f1100
to
4086036
Compare
I do not have any ideia why Cypress tests is not passing anymore... |
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
4086036
to
82aeb06
Compare
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
82aeb06
to
3ff23c7
Compare
There was a problem hiding this 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 :)
Tested with deck and main bundle size went down in dev build from 9.9 to 7.3 MB 🥳 |
Signed-off-by: Vinicius Reis <vinicius.reis@nextcloud.com>
@@ -30,6 +31,7 @@ | |||
"cypress:update-snapshots": "cypress run-ct --env type=base --config screenshotsFolder=cypress/snapshots/base" | |||
}, | |||
"main": "dist/ncvuecomponents.js", | |||
"module": "dist/index.module.js", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 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 |
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
So, will be some cases as the bundle will increase until we fully migrate to another solution like vite, and solve those problems. |
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. |
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.We already are able to use a different approach.
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'
andinstall
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 generateindex.module.js
.Break Changes
Emoji functions
As all functions are directly expose, the function
addRecent
fromsrc/functions/emoji
have chanced to make clear what kind of function it is.Now
addRecent
is calledemojiAddRecent
This is related to #2959.
Footnotes
webpack, Roolup and parcel ↩