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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: preCloseBundle hook #9326

Conversation

userquin
Copy link
Contributor

@userquin userquin commented Jul 23, 2022

Description

Add a new preCloseBundle hook to allow run build plugins hooks between Rollup writeBundle and closeBundle hooks.

EDIT: we need it also in Vite 2 馃檹 .

I changed also scripts/patchCJS.ts module to allow build in Windows: I just change process.exit() call with process.exit(0): sent another PR for this.

Additional context

Trying to integrate with SvelteKit the vite-plugin-pwa plugin, we have found a problem about a race condition using both plugins with Vite 3 (also with Vite 2, right now latest SvelteKit version will work only with Vite 3).

SvelteKit uses the writeBundle to call the prerender process (SSG). In the closeBundle hook, it calls the adapter logic, the adapter gets the prerender output and copy the content to the adapter output folder (it can do more things).

vite-plugin-pwa uses closeBundle hook in its build plugin to generate the service worker, it has enforce: ' post', and so will be called by Vite/Rollup after SvelteKit closeBundle hook.

We need to invert the closeBundle hook call, that's, vite-plugin-pwa first and then SvelteKit`.

We can reverse the order just changing the vite-plugin-pwa build plugin with enforce: 'pre', but then we have a race condition: since the closeBundle hook will be called in parallel, both plugins need to be called in a sequential order; why? because the adapter needs to copy also the service worker generated by the vite-plugin-pwa build plugin.

Can we move the vite-plugin-pwa build plugin to the writeBundle hook: the answer is no; the vite-plugin-pwa will need the prerender output, and so we have the another race condition, since SvelteKit and vite-plugin-pwa build plugin will run in parallel.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@userquin
Copy link
Contributor Author

sorry: wrong changelist selected in my IDE, fixing lint

@dominikg
Copy link
Contributor

the problem this solves is basically that two plugins both use parallel hooks but these hooks depend on each other.
Introducing a new hook in between solves this particular issue, but it would need more discussion, documentation and comes with a lot of added maintenance/testing and risks of incompatibility with future updates to rollup, so i don't think this would be a great addition in it's current form.

Alternative suggestions (some of which already have been discussed in chat):

1) the plugins depending on each other find a way to signal each other via plugin.api

e.g. plugin-a offers plugin.api.on('someevent', ()=>{}) and plugin-b goes plugins.find(p=>p.name === 'plugin-b')?.api.on('someevent',()=>{resolvePromiseYourHookWaitsOn}

2) enforce 'pre' and 'post' in vite are changed to also apply to parallel hook execution

Instead of running all hooks in parallel it would first run all 'pre' hooks in parallel, then the regular hooks and then 'post' hooks.
This is essentially a more generic and less invasive option as is gives every parallel hook a 'pre' and 'post' phase instead of just adding one extra hook in between the two that are causing the root issue this PR is trying to tackle.

3) use a separate vite plugin to control the execution of parallel hooks

This is addmittedly a hackish solution, but gives you total control without vite changing anything. A separate plugin vite-plugin-hook-execution-order can be used to change the plugin hooks at config time.
Eg. rename all plugin.closeBundle to plugin._closeBundle, register a single closeBundle hook that awaits on each of plugin._closeBundle in the order you need.

There are multiple ways to implement this, it could be used to implement the phased parallel logic outlined in 2) outside of vite core, receive custom configuration so that users can change the order as needed, or be supplied with hardcoded sorting by the plugin that relies on a particular order. (Though the last one would get ugly if two plugins had that idea).

--

This comment is not meant to disagree with adding new hook(s) in general, just that doing so requires more up front discussion and presentation of usecases. I guess prerendering in particular isn't something that rollup had in mind when it was designing it's hooks, so new ones may be warranted at some point if that area is something vite wants to cover on top of bundling itself.

@userquin
Copy link
Contributor Author

This PR is about the sequential plugin hack, I just renamed it so it doesn't sound drastic ;) .

I guess Rollup should add configurable hooks, points 2) and 3) and this PR have the same problem, we need to hack Rollup.

Point 3) I think it is almost impossible to implement it in the current state

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

Successfully merging this pull request may close these issues.

None yet

2 participants