-
Notifications
You must be signed in to change notification settings - Fork 541
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
fix(loader): clone preset object #2983
Conversation
We should rebase it to v2 |
@huang-julien can you please rebase your PR against v2 branch? |
ab6b3af
to
70e1629
Compare
@huang-julien Thanks for PR. as this change adds runtime cost (and there is no easy way to reproduce/confirm with nitro only) can you please try on nightly channel if it actually solved linked issues? |
Thanks for merging this so fast β€οΈ https://github.com/nuxt/nuxt/tree/repro/nitro-preset-shared-object here's the branch for reproduction in Nuxt. You need to run It doesn't solve linked issues but allows to create the PR that will solve them in Nuxt. |
That's a Nuxt reproduction. it was generally good safety fix thanks β€οΈ |
ahhh so sorry about that https://stackblitz.com/edit/unjs-nitro-starter-qxgcfv55?file=createNitro.ts&title=Nitro%20Starter you can run |
π Linked issue
nuxt/test-utils#537
nuxt/test-utils#1043 ?
β Type of change
Hi π
This PR fix an issue with presets and will allow multiple Nuxt to be ran within the same process (like for vitest workspaces or its vscode extension)
Linked issues above are trying to run multiple nuxt at the same time. It means we need also to be able to start multiple nitro.
Here's the race condition.
Presets are objects and
nitro/src/core/config/loader.ts
Line 141 in e8d6099
returns directly the reference of the preset.
If we load 2 nitro at the same time, C12 can delete _layers on the preset object
https://github.com/unjs/c12/blob/67cf916ca727e584390196ce3d3bfb76aa102ec1/src/loader.ts#L267
and the other other may need _layer at that time (calling .push() for example) which will fail and throw an error.
(will this pr be merge in nitro 2 or does it need another pr ? )
π Description
π Checklist