Repository navigation
fix(build): keep the DAV properties apart from the webdav client - #1792
Conversation
skjnldsv
added
type: bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1792 +/- ##
=======================================
Coverage 89.86% 89.86%
=======================================
Files 44 44
Lines 1135 1135
Branches 285 285
=======================================
Hits 1020 1020
Misses 85 85
Partials 30 30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Waiting for @susnux |
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
I do not get this changes, this enforces a specific output files for dav properties.
But that will not change anything, because its not an entry chunk the apps bundler will still load everything else.
So tree shaking will be applied and everything not needed is dropped again.
The sideeffects part is ok, but the manual chunks part? Also why a plugin? There is a build option for it
|
Well, I tried a few things and discovered a mix of options that seemed to be needed together. as you can see in the table in the description.
I wasn't aware of an option for that directly. Searching I found this plugin 🙈🙈 The ultimate goal here is being able to use the |
| // The DAV properties apart from the client, so an app that only | ||
| // registers a property does not bundle webdav where it does | ||
| name: 'dav-properties-chunk', | ||
| outputOptions(options) { | ||
| return { | ||
| ...options, | ||
| manualChunks(id) { | ||
| if (id.includes('/lib/dav/davProperties.ts')) { | ||
| return 'davProperties' | ||
| } | ||
| }, | ||
| } | ||
| }, |
There was a problem hiding this comment.
| // The DAV properties apart from the client, so an app that only | |
| // registers a property does not bundle webdav where it does | |
| name: 'dav-properties-chunk', | |
| outputOptions(options) { | |
| return { | |
| ...options, | |
| manualChunks(id) { | |
| if (id.includes('/lib/dav/davProperties.ts')) { | |
| return 'davProperties' | |
| } | |
| }, | |
| } | |
| }, | |
| { | |
| name: 'preserve-modules', | |
| outputOptions: (output) => ({ ...output, preserveModules: true, preserveModulesRoot: 'lib' }), | |
| }, |
I guess this is future proof as not bound to a specific input file
There was a problem hiding this comment.
(alternatively we might also just drop vite here because this lib does not need to bundle things (e.g. Vue components) but only simply provides vanilla function, thus a Typescript build with ts 7 should be simpler and faster)
There was a problem hiding this comment.
Fine with any solution really :)
There was a problem hiding this comment.
Seems like it's working!
Pushed the fix 😊
|
If you meant build.rollupOptions.output.manualChunks, I tried it first. But createLibConfig defines rollupOptions.output as an array (one entry per format), and Vite's mergeConfig appended my object as a second output instead of merging into it 😵💫 |
The DAV properties and the client were built into one module, which a bundler can tree-shake but not split. An app registering a property at init and using the client in a lazy chunk got the whole module, webdav included, in a chunk its page loads at once. Each source module is now a module of its own, and the package says it has no side effects, so a bundler places each where it is used. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: John Molakvoæ <14975046+skjnldsv@users.noreply.github.com>
b52c9f0 to
2f79583
Compare
|
TODO for us: |
|
(while there partially they have sideeffects - we should look into fixing that to be sideeffect free) |
3 participants
Importing
registerDavPropertyfrom@nextcloud/files/davbrings the whole webdav client into the importing chunk. The properties and the client were built into one chunk, so an app that registers a property at startup and uses the client later, lazily, gets the client in the chunk its page loads at once.Measured on the viewer's playground, registering
nc:metadata-photos-sizefrom its init:@nextcloud/filessideEffects: falseonlyEach source module is now its own module (
preserveModules, as susnux suggested), and the package says it has no side effects, so a bundler places each where it is used. Both are needed. The package ships no CSS, and its two module-level statements (the global scope andaxiosRetry) sit in modules that are kept whenever their exports are used.Unit tests pass locally, the integration ones need Docker and are left to CI.
👾 This pull request was assisted by Claude Code, commits carry an
Assisted-bytrailer.