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

Migrate build of single entry points from webpack to vite #4386

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Aug 1, 2023

With this PR all dist files will be built using Vite, see todo for rest of webpack.

☑️ Resolves

(previously this PR was breaking, but this is changed):

⚠️ This is a breaking change! This simply adds more entry points, which is good as now code can be shared between the entry points which allows for smaller app bundles. But as this also introduces ESM entry points for all components it changes the file extensions from `.js` to `.cjs` and `.mjs` respective.

Because of this I think we should take the opportunity and move from /dist/Components/... to simply /components/....
This will also make it easier when we decide to provide Typescript definitions as then the same layout of dist as of src is used.

So this means you will now need to change the imports:

// Instead of this:
import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'

// Use now
import NcButton from '@nextcloud/vue/components/NcButton.js'

Benchmark

If you need a reason why this is useful:
text js bundle size decreases by 4% (not much because text mostly imports from main entry point)
but
logreader js bundle size decreases by 27% (-600KiB!).

Migration guide

sed -Ei \
    -e 's|@nextcloud/vue/dist/Components/([^\.]+)\.js|@nextcloud/vue/components/\1|' \
    -e 's|@nextcloud/vue/dist/Directives/([^\.]+)\.js|@nextcloud/vue/directives/\1|' \
    -e 's|@nextcloud/vue/dist/Functions/([^\.]+)\.js|@nextcloud/vue/functions/\1|' \
    -e 's|@nextcloud/vue/dist/Mixins/([^\.]+)\.js|@nextcloud/vue/mixins/\1|' \
    src/**/*.js src/**/*.vue

🚧 Tasks

Webpack is still used by:

  • styleguide (either use vue-docgen + vitepress or consider moving to industry standard storybook)
  • Cypress (would do a follow up)

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable

@susnux susnux force-pushed the feat/migrate-build-vite branch from b6c2e9a to 87db1e5 Compare August 2, 2023 10:24
@skjnldsv skjnldsv closed this Aug 2, 2023
@skjnldsv skjnldsv reopened this Aug 2, 2023
@skjnldsv
Copy link
Contributor

skjnldsv commented Aug 2, 2023

Wrong button.....

@susnux can you fix the docs import pattern too? :)

EDIT: I can do it myself, that should be faster 🙈, sorry for the ping!

@skjnldsv

This comment was marked as outdated.

@skjnldsv skjnldsv enabled auto-merge August 2, 2023 15:04
@susnux

This comment was marked as outdated.

@skjnldsv

This comment was marked as outdated.

@susnux

This comment was marked as outdated.

@skjnldsv

This comment was marked as outdated.

@susnux
Copy link
Contributor Author

susnux commented Aug 4, 2023

PR for rollup: rollup/plugins#1549

Currently I think only the nextcloud-upload library uses rollup? Maybe just migrate to vite there and use the js extension here.

package.json Outdated Show resolved Hide resolved
@susnux
Copy link
Contributor Author

susnux commented Aug 5, 2023

@skjnldsv @ShGKme I pushed a commit making it non breaking, what do you think?

@skjnldsv
Copy link
Contributor

skjnldsv commented Aug 5, 2023

@skjnldsv @ShGKme I pushed a commit making it non breaking, what do you think?

Also good from me :)

@susnux susnux requested a review from ShGKme August 7, 2023 12:02
@susnux susnux force-pushed the feat/migrate-build-vite branch 3 times, most recently from 4216447 to 04ba1d6 Compare August 16, 2023 21:22
@susnux
Copy link
Contributor Author

susnux commented Aug 16, 2023

@ShGKme what do you think? Would be good for #4415

@susnux
Copy link
Contributor Author

susnux commented Sep 14, 2023

ping @ShGKme @raimund-schluessler

This is a breaking change as this also introduces ESM entry points for all components etc
and therfor changed the files from `.js` to `.cjs` and `.mjs` respective.

Instead of `import NcButton from '@nextcloud/vue/dist/Components/NcButton.js` use `import NcButton from '@nextcloud/vue/components/NcButton'`.

* Also fix docs for component import pattern

Co-authored-by: John Molakvoæ <skjnldsv@users.noreply.github.com>

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Seems to work fine and looks fine by configs.

Am I right that now styles are not loaded from .js but loaded by require('path-to.css') inside modules?

image

var r=require("../assets/index45.css");const a=require("../chunks/actionText-bef01778.cjs"),

I'm a bit afraid of it because it is not a correct import/require for module systems, and works only with bundlers. Won't it be broken when dependencies (/node_modules/) are ignored for css rules or in unit testing (except Vitest)?

P.S. I'm also not fun of putting CSS inside JS as it was before, because it is much worse from a performance perspective. An alternative could be to bundle css in separate, but it would be a breaking change requiring developers to import css implicitly with the component.

@skjnldsv skjnldsv merged commit 9ac3a78 into master Sep 14, 2023
@skjnldsv skjnldsv deleted the feat/migrate-build-vite branch September 14, 2023 11:47
@Antreesy
Copy link
Contributor

Antreesy commented Oct 12, 2023

Am I right that now styles are not loaded from .js but loaded by require('path-to.css') inside modules?

Might be related to it?

  • with import of NcRichContenteditable to the project also the component NcMentionBubble is bundled with id '357e6d0e' (which will be used for styles scope in [data-v-357e6d0e])
  • NcRichContenteditable is using mixin richEditor under the hood, which is in charge of rendering NcMentionBubble. It is also bundled in the project with id '8a961b36'
  • dist/assets/NcMentionBubble-<hash>.css contains styles for scope data-v-357e6d0e, same as source webpack://./node_modules/@nextcloud/vue/dist/assets/index.css in production code
  • dev environment seems to use mjs, where styles applied only for [data-v-8a961b36]

That causes the styles for rendered mentions to be broken, if lib is used in Talk, for example:
image

Content in a bundled app:

  1. NcMentionBundle with id '357e6d0e'
  2. Styles for id '8a961b36'
  3. NcMentionBundle with id '8a961b36'
    Screenshot from 2023-10-12 10-26-42

@ShGKme
Copy link
Contributor

ShGKme commented Oct 12, 2023

A small note about the problem: NcMentionBubble is the only Vue component that is imported from a .js file.

The css chunk is created in development mode, but for some reason is not imported by the mixin chunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants