Description
Paul Derscheid
2024-09-12 19:04:44 UTC
Created attachment 171425 [details] [review] Bug 37911: Prototype vue islands within static pages This is pretty bare bones, but might still be interesting to take a look at. Created attachment 171426 [details] [review] Bug 37911: (follow-up) Correct wishful thinking in displayed comment about changes to data-props Created attachment 171432 [details] [review] Bug 37911: (follow-up) Update to vue ^3.5.4, use vue's web component implementation instead - This would require us to update vue to a new minor version. - We could then use the defineCustomElement function with the shadowRoot option set to false (this is important). - The current implementation in islands.ts is naive and would result in an intolerable bundle size. - The right way to do code splitting here is to use dynamic imports. Vue ^3.5.x provides the perfect way to do this! Was too tired to get the dynamic imports right, but if you apply all three patches you'll see what magic could await us :) Created attachment 171439 [details] [review] Bug 37911: (follow-up) Experimental introduction of dynamic imports for code splitting This looks great! I think the vue upgrade should 100% be done. When I tried this a while ago the shadowRoot option was exactly what I was missing - couldn't get any core CSS classes to work in the components Code splitting seems to work fine, I can see separate bundles for multiple islands (I created some extras to test). Its a shame there doesn't seem to be a `defineAsyncCustomElement` method so that we aren't loading all the bundles on each page even if we aren't using some of the islands Thanks for testing it Matt! There are some problems currently: - We need to define a way to provide the dependencies for the preexisting shared components (maybe you'd like to experiment on that one). - As you pointed out, we need to lazy load what's needed (currently on that one). - The chunk names are annoying (patch incoming). Created attachment 171440 [details] [review] Bug 37911: (follow-up) Use webpack import annotations for more concise chunk names Created attachment 171445 [details] [review] Bug 37911: (follow-up) Scan for requested components in caller and lazy load This should ensure that we only load what's requested. Maybe we can also find a way to call the exported hydrate function on demand. To test: 1) Apply the patch 2) Run yarn js:watch and wait for the first build if necessary 3) Load mainpage.pl and note that only hello-islands.ts is imported. 4) Uncomment <hello-world></hello-world> in intranet-main.tt 5) Reload the page 6) Note that now both component chunks are imported on demand 7) Discuss Typo, should say hello-islands.js, not .ts. Created attachment 171459 [details] [review] Bug 37911: (follow-up) Prevent resolution failure of dynamic imports when building in mode --production Created attachment 171517 [details] [review] Bug 37911: (follow-up) Enable usage of dependencies Created attachment 171538 [details] [review] Bug 37911: (follow-up) Move store initialisation into hydrate's idle callback - Additionally: - Use a type instead of an interface. - Add JSDoc to it. - Use optional chaining when checking for existence and length of config. Created attachment 171608 [details] [review] Bug 37911: (follow-up) Prevent resolution failure of dynamic imports when building in mode --production Created attachment 171685 [details] [review] Bug 37911: (follow-up) Allow for initialisation of islands on demand - Split the rspack config into a umd and an esm section. - Don't hydrate by default. - Instead check the "init" attribute for the currentScript when using the umd. - Allow for manual import and call when using the esm. - Add examples for both in intranet-main.tt (to be removed). - Add a DialogIsland wrapper for the shared Dialog component (to map props to stored refs). - Move DialogIsland as well as Demo components into an Islands directory under components. - Add i18n initialisation to hydrate's idle callback. Created attachment 172962 [details] [review] Bug 37911: (follow-up) Remove example islands for core submission Created attachment 172963 [details] [review] Bug 37911: (follow-up) Remove example islands for core submission Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Created attachment 172965 [details] [review] Bug 37911: Prototype vue islands within static pages This is pretty bare bones, but might still be interesting to take a look at. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172966 [details] [review] Bug 37911: (follow-up) Correct wishful thinking in displayed comment about changes to data-props Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172967 [details] [review] Bug 37911: (follow-up) Update to vue ^3.5.4, use vue's web component implementation instead - This would require us to update vue to a new minor version. - We could then use the defineCustomElement function with the shadowRoot option set to false (this is important). - The current implementation in islands.ts is naive and would result in an intolerable bundle size. - The right way to do code splitting here is to use dynamic imports. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172968 [details] [review] Bug 37911: (follow-up) Experimental introduction of dynamic imports for code splitting Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172969 [details] [review] Bug 37911: (follow-up) Use webpack import annotations for more concise chunk names Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172970 [details] [review] Bug 37911: (follow-up) Scan for requested components in caller and lazy load This should ensure that we only load what's requested. Maybe we can also find a way to call the exported hydrate function on demand. To test: 1) Apply the patch 2) Run yarn js:watch and wait for the first build if necessary 3) Load mainpage.pl and note that only hello-islands.ts is imported. 4) Uncomment <hello-world></hello-world> in intranet-main.tt 5) Reload the page 6) Note that now both component chunks are imported on demand 7) Discuss Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172971 [details] [review] Bug 37911: (follow-up) Enable usage of dependencies Created attachment 172972 [details] [review] Bug 37911: (follow-up) Move store initialisation into hydrate's idle callback - Additionally: - Use a type instead of an interface. - Add JSDoc to it. - Use optional chaining when checking for existence and length of config. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172973 [details] [review] Bug 37911: (follow-up) Prevent resolution failure of dynamic imports when building in mode --production Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172974 [details] [review] Bug 37911: (follow-up) Allow for initialisation of islands on demand - Split the rspack config into a umd and an esm section. - Don't hydrate by default. - Instead check the "init" attribute for the currentScript when using the umd. - Allow for manual import and call when using the esm. - Add examples for both in intranet-main.tt (to be removed). - Add a DialogIsland wrapper for the shared Dialog component (to map props to stored refs). - Move DialogIsland as well as Demo components into an Islands directory under components. - Add i18n initialisation to hydrate's idle callback. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 172975 [details] [review] Bug 37911: (follow-up) Remove example islands for core submission Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Are you/we going to use it? I don't think we should include it if there are no follow-up plans. I have a prototype in bug 38010 - it allows the acquisitions left menu to be migrated to Vue and used in a Vue app as well as in template files where required Applying: Bug 37911: Prototype vue islands within static pages Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt M rspack.config.js Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt error: Failed to merge in the changes. Patch failed at 0001 Bug 37911: Prototype vue islands within static pages I'm trying to fix as I go, but I keep getting more and more conflicts. Please rebase and squash where possible. (In reply to Jonathan Druart from comment #29) > Are you/we going to use it? I don't think we should include it if there are > no follow-up plans. It's too difficult to review using Splinter Review and I can't apply the patches, so I can't say for sure yet....but for a long time I've been asking us to bundle the Vue.js differently, so that we can use Vue.js for embedded web components to embed Vue components throughout other parts of Koha and in Koha plugins. I'd love for everyone to be across the Vue.js work, and for us to start moving ahead further with it. Created attachment 176959 [details] [review] Bug 37911: Prototype vue islands within static pages This is pretty bare bones, but might still be interesting to take a look at. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176960 [details] [review] Bug 37911: (follow-up) Correct wishful thinking in displayed comment about changes to data-props Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176961 [details] [review] Bug 37911: (follow-up) Update to vue ^3.5.4, use vue's web component implementation instead - This would require us to update vue to a new minor version. - We could then use the defineCustomElement function with the shadowRoot option set to false (this is important). - The current implementation in islands.ts is naive and would result in an intolerable bundle size. - The right way to do code splitting here is to use dynamic imports. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176962 [details] [review] Bug 37911: (follow-up) Experimental introduction of dynamic imports for code splitting Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176963 [details] [review] Bug 37911: (follow-up) Use webpack import annotations for more concise chunk names Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176964 [details] [review] Bug 37911: (follow-up) Scan for requested components in caller and lazy load This should ensure that we only load what's requested. Maybe we can also find a way to call the exported hydrate function on demand. To test: 1) Apply the patch 2) Run yarn js:watch and wait for the first build if necessary 3) Load mainpage.pl and note that only hello-islands.ts is imported. 4) Uncomment <hello-world></hello-world> in intranet-main.tt 5) Reload the page 6) Note that now both component chunks are imported on demand 7) Discuss Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176965 [details] [review] Bug 37911: (follow-up) Enable usage of dependencies Created attachment 176966 [details] [review] Bug 37911: (follow-up) Move store initialisation into hydrate's idle callback - Additionally: - Use a type instead of an interface. - Add JSDoc to it. - Use optional chaining when checking for existence and length of config. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176967 [details] [review] Bug 37911: (follow-up) Prevent resolution failure of dynamic imports when building in mode --production Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176968 [details] [review] Bug 37911: (follow-up) Allow for initialisation of islands on demand - Split the rspack config into a umd and an esm section. - Don't hydrate by default. - Instead check the "init" attribute for the currentScript when using the umd. - Allow for manual import and call when using the esm. - Add examples for both in intranet-main.tt (to be removed). - Add a DialogIsland wrapper for the shared Dialog component (to map props to stored refs). - Move DialogIsland as well as Demo components into an Islands directory under components. - Add i18n initialisation to hydrate's idle callback. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 176969 [details] [review] Bug 37911: (follow-up) Remove example islands for core submission Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Created attachment 176970 [details] [review] Bug 37911: (follow-up) Isolate yarn.lock changes Looks like the yarn.lock file was causing conflicts due to version changes since this was uploaded. I've isolated it into its own commit to make rebasing easier and allow the RM to decide whether to push that commit or drop it and generate a fresh lock file at the time of pushing Created attachment 176972 [details] [review] Bug 37911: (follow-up) Isolate yarn.lock changes Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> |