-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
feat: allow multiple nuxt in a single process #30510
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected] |
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ starter, example-layers-monorepo, nuxt-com, cli, tailwindcss |
ecosystem ci fails due to typecheck |
https://github.com/nuxt/ecosystem-ci/actions/runs/12725855624 ecosystem ci for nuxt 3 |
return nuxtCtx.tryUse() | ||
const nuxt = nuxtCtx().tryUse() | ||
if (!nuxt) { | ||
logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v4.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to rephrase this to make it sound a bit gentler 😅. Any suggestions?
cc @danielroe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also isn't going to be remove d in V4, maybe in V5 🤔
WalkthroughThe pull request introduces significant changes to the Nuxt context management system across multiple packages. The primary modifications focus on enhancing context retrieval and management using asynchronous local storage. A new The changes introduce a unique identifier ( The modifications include updates to context retrieval methods, fallback mechanisms for global context usage, and improved logging for deprecated usage patterns. These changes aim to provide more flexible and reliable context management across different Nuxt operations, with a focus on asynchronous execution and context tracking. Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/kit/src/context.ts (1)
26-26
: 🛠️ Refactor suggestionRephrase the deprecation warning for clarity and correct version
The warning message could be phrased more gently and should reflect the correct version in which the deprecation will occur.
Suggested change:
-logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behaviour is deprecated and will be removed in v4.') +logger.warn('Using fallback global Nuxt instance. Using @nuxt/kit composables outside of a Nuxt context is deprecated and will be removed in v5. Please ensure you are within a proper Nuxt context.')Also applies to: 50-50
🧹 Nitpick comments (3)
packages/nuxt/test/load-nuxt.test.ts (1)
47-56
: Enhance test coverage for multiple Nuxt instances.Whilst the test verifies concurrent loading, it could be more robust with:
- Assertions to verify each instance has a unique
__name
- Verification that each instance operates independently
- Cleanup of instances after the test
it('load multiple nuxt', async () => { - await Promise.all([ + const [nuxt1, nuxt2] = await Promise.all([ loadNuxt({ cwd: repoRoot, }), loadNuxt({ cwd: repoRoot, }), ]) + expect(nuxt1.__name).toBeDefined() + expect(nuxt2.__name).toBeDefined() + expect(nuxt1.__name).not.toBe(nuxt2.__name) + await Promise.all([nuxt1.close(), nuxt2.close()]) })packages/nuxt/src/core/nuxt.ts (2)
54-54
: Consider documenting the new__name
property.The new
__name
property and its usage withasyncNameStorage
should be documented to help users understand how multiple Nuxt instances are managed.Would you like me to generate documentation for the new
__name
property and its role in multi-instance support?Also applies to: 62-62, 66-66
178-189
: Enhance the backward compatibility comment.The comment "backward compatibility with 3.x" could be more descriptive to explain why and how the global singleton is maintained.
Consider updating the comment to:
- // backward compatibility with 3.x + // Maintain backward compatibility with Nuxt 3.x by setting the first instance as global singleton. + // This ensures existing applications continue to work without modifications.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
,!pnpm-lock.yaml
📒 Files selected for processing (8)
packages/kit/src/context.ts
(3 hunks)packages/kit/src/index.ts
(2 hunks)packages/kit/src/loader/nuxt.ts
(2 hunks)packages/kit/src/utils.ts
(1 hunks)packages/nuxt/src/core/nuxt.ts
(4 hunks)packages/nuxt/test/load-nuxt.test.ts
(1 hunks)packages/schema/src/types/nuxt.ts
(1 hunks)test/bundle.test.ts
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (6)
packages/kit/src/loader/nuxt.ts (1)
44-44
: Ensurenuxt.__name
is defined before using it inasyncNameStorage.run
If
nuxt.__name
is undefined or not set, callingasyncNameStorage.run(nuxt.__name, ...)
may cause unexpected behaviour. Please verify that__name
is properly initialised on thenuxt
instance.You might consider adding a default name or handling the undefined case:
-return asyncNameStorage.run(nuxt.__name, () => build(nuxt)) +const name = nuxt.__name || 'default-nuxt-instance' +return asyncNameStorage.run(name, () => build(nuxt))packages/kit/src/index.ts (2)
21-21
: LGTM! Context management exports look good.The renaming of
nuxtCtx
togetNuxtCtx
and addition ofglobalNuxtCtx
properly support both the new multi-instance capability and maintain backward compatibility.
37-37
: LGTM! Async storage export enables instance tracking.The export of
asyncNameStorage
aligns with the PR's objective of managing multiple Nuxt instances in a single process.packages/schema/src/types/nuxt.ts (1)
83-86
: LGTM! Well-documented interface addition.The
__name
property is:
- Properly marked as internal with double underscore prefix
- Well-documented with clear use cases
- Correctly typed as string
test/bundle.test.ts (1)
103-103
: LGTM! Acceptable bundle size increase.The 1KB increase in server bundle size is minimal and justified by the addition of multi-instance support functionality.
packages/nuxt/src/core/nuxt.ts (1)
7-7
: LGTM! Import changes look good.The new imports support the multi-instance functionality with proper context management.
Also applies to: 25-25
/** Direct access to the Nuxt context - see https://github.com/unjs/unctx. */ | ||
export const nuxtCtx = getContext<Nuxt>('nuxt') | ||
/** Direct access to the Nuxt context with asyncLocalStorage - see https://github.com/unjs/unctx. */ | ||
export const getNuxtCtx = () => getContext<Nuxt>(asyncNameStorage.getStore()!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle potential null
return value from asyncNameStorage.getStore()
Using the non-null assertion operator !
with asyncNameStorage.getStore()
may lead to runtime errors if the store is undefined
. Consider adding a null check to ensure robustness.
Suggested change:
-export const getNuxtCtx = () => getContext<Nuxt>(asyncNameStorage.getStore()!)
+export const getNuxtCtx = () => {
+ const store = asyncNameStorage.getStore()
+ if (!store) {
+ // Handle the undefined case, e.g., throw an error or provide a fallback
+ throw new Error('AsyncLocalStorage store is undefined')
+ }
+ return getContext<Nuxt>(store)
+}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export const getNuxtCtx = () => getContext<Nuxt>(asyncNameStorage.getStore()!) | |
export const getNuxtCtx = () => { | |
const store = asyncNameStorage.getStore() | |
if (!store) { | |
// Handle the undefined case, e.g., throw an error or provide a fallback | |
throw new Error('AsyncLocalStorage store is undefined') | |
} | |
return getContext<Nuxt>(store) | |
} |
@@ -50,18 +51,19 @@ import { VirtualFSPlugin } from './plugins/virtual' | |||
|
|||
export function createNuxt (options: NuxtOptions): Nuxt { | |||
const hooks = createHooks<NuxtHooks>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably need to ensure that hooks are called in a way that passes the name of the Nuxt concerned.... maybe via a custom caller, or by overriding callHook
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hooks is assigned to the Nuxt instance.
If users wants to know which nuxt is concerned, they probably knows it when calling useNuxt
or nuxt.hook
<-- means they have stored it in a variable somewhere
What are you thinking of 👀 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we want this to work:
nuxt.hook('modules:done', () => {
addTemplate(someTemplate)
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually that may be a bad example as it's called within initNuxt
but consider another hook that's called later on - we need nuxt context available then too. Users shouldn't have to update their code to explicitly run nuxt.run
any time they need to run a kit utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
const nuxt: Nuxt = {
_version: version,
options,
hooks,
callHook: hooks.callHook,
addHooks: hooks.addHooks,
hook: (name, fn, opts) => asyncNameStorage.run(name, () => hooks.hook(name, fn, opts)),
ready: () => asyncNameStorage.run(name, () => initNuxt(nuxt)),
close: () => hooks.callHook('close', nuxt),
vfs: {},
apps: {},
__name: name,
}
this would only work in hooks tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd still need to add a nuxt.run
in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also possible to acess nuxt.hooks
directly, and in fact we use addHooks
to add user hooks, for example.
we can have nuxt.run
, of course, but the value of most of kit utilities is that they shouldn't require it by having to pass nuxt
around. at least reducing the surface area of possible places where users can call it would help a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also possible to acess nuxt.hooks directly, and in fact we use addHooks to add user hooks, for example.
Indeed so do we need to override it directly in hooks
? Or could we maybe override callHook
from hooks
we can have nuxt.run, of course, but the value of most of kit utilities is that they shouldn't require it by having to pass nuxt around. at least reducing the surface area of possible places where users can call it would help a lot.
It's only for cases when users wants to run it outside of loadNuxt
or buildNuxt
, I don't really see a lot of cases 🤔
…into feat/nuxt_async_context
Co-authored-by: Daniel Roe <[email protected]>
… run fn and wrap callhooks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/kit/src/context.ts (1)
6-7
:⚠️ Potential issueHandle potential
null
return value fromasyncNameStorage.getStore()
Using the non-null assertion operator
!
withasyncNameStorage.getStore()
may lead to runtime errors if the store isundefined
. Consider adding a null check to ensure robustness.-export const getNuxtCtx = () => getContext<Nuxt>(asyncNameStorage.getStore()!) +export const getNuxtCtx = () => { + const store = asyncNameStorage.getStore() + if (!store) { + // Handle the undefined case, e.g., throw an error or provide a fallback + throw new Error('AsyncLocalStorage store is undefined') + } + return getContext<Nuxt>(store) +}
🧹 Nitpick comments (4)
packages/nuxt/test/load-nuxt.test.ts (2)
52-62
: Consider enhancing test coverageWhile the test correctly verifies concurrent loading and absence of warnings, consider adding assertions to:
- Verify each instance has a unique
__name
- Confirm each instance is properly initialised
- Test the context isolation between instances
it('load multiple nuxt', async () => { - await Promise.all([ + const [nuxt1, nuxt2] = await Promise.all([ loadNuxt({ cwd: repoRoot, }), loadNuxt({ cwd: repoRoot, }), ]) expect(loggerWarn).not.toHaveBeenCalled() + expect(nuxt1.__name).not.toBe(nuxt2.__name) + expect(nuxt1).toBeDefined() + expect(nuxt2).toBeDefined() + + // Test context isolation + await nuxt1.run(() => { + expect(asyncNameStorage.getStore()).toBe(nuxt1.__name) + }) + await nuxt2.run(() => { + expect(asyncNameStorage.getStore()).toBe(nuxt2.__name) + }) })
64-81
: Improve type safety in testInstead of using
@ts-expect-error
for a "random" hook, consider:
- Using a properly typed test hook
- Creating a type-safe test hook name constant
+const TEST_HOOK = 'test:context' as const + it('expect hooks to get the correct context outside of initNuxt', async () => { const nuxt = await loadNuxt({ cwd: repoRoot, }) - // @ts-expect-error - random hook - await nuxt.hook('test', () => { + await nuxt.hook(TEST_HOOK, () => { const nuxt = useNuxt() expect(asyncNameStorage.getStore()).toBe(nuxt.__name) }) expect(asyncNameStorage.getStore()).toBeUndefined() - // @ts-expect-error - random hook - await nuxt.callHook('test') + await nuxt.callHook(TEST_HOOK) expect(loggerWarn).not.toHaveBeenCalled() })packages/schema/src/types/nuxt.ts (1)
90-93
: Enhance method documentationConsider adding more detailed documentation for the
run
method to explain:
- Its purpose in managing async context
- Usage examples
- Return value expectations
/** * @internal + * Executes a function within this Nuxt instance's context. + * + * @example + * ```ts + * await nuxt.run(async () => { + * const ctx = useNuxt() // Gets this instance's context + * await ctx.callHook('my-hook') + * }) + * ``` + * + * @param fn The function to execute within this context + * @returns The return value of the executed function */ run: <T extends (...args: any[]) => any>(fn: T) => ReturnType<T>packages/nuxt/src/core/nuxt.ts (1)
83-89
: Consider adding error handlingThe context setup could benefit from error handling to ensure proper cleanup in case of initialization failures.
nuxt.run(() => { + try { // Set nuxt instance for useNuxt getNuxtCtx().set(nuxt) nuxt.hook('close', () => { getNuxtCtx().unset() }) + } catch (error) { + logger.error('Failed to initialize Nuxt context:', error) + throw error + } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
,!pnpm-lock.yaml
📒 Files selected for processing (6)
packages/kit/src/context.ts
(3 hunks)packages/kit/src/index.ts
(2 hunks)packages/kit/src/utils.ts
(1 hunks)packages/nuxt/src/core/nuxt.ts
(4 hunks)packages/nuxt/test/load-nuxt.test.ts
(4 hunks)packages/schema/src/types/nuxt.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/kit/src/utils.ts
- packages/kit/src/index.ts
🔇 Additional comments (9)
packages/kit/src/context.ts (1)
3-3
: LGTM! The import ofasyncNameStorage
is correctly added for the new context management system.packages/nuxt/test/load-nuxt.test.ts (2)
7-7
: LGTM! The required imports are correctly added for the new test cases.
28-30
: LGTM! ThebeforeEach
hook correctly clears theloggerWarn
spy, ensuring a clean state for each test.packages/schema/src/types/nuxt.ts (1)
83-86
: LGTM! The__name
property is well-documented and correctly typed.packages/nuxt/src/core/nuxt.ts (5)
3-3
: LGTM! Using the built-inrandomUUID
fromnode:crypto
is a good choice for generating unique instance identifiers.
56-59
: LGTM! Hook calls are correctly wrapped to maintain context, covering all hook methods (callHook
,callHookParallel
,callHookWith
).
68-74
: LGTM! The Nuxt instance is correctly configured with the new__name
property,run
method, and context-awareready
method.
76-82
: LGTM! The global context fallback is correctly implemented with proper cleanup via theclose
hook.
824-837
: LGTM! The dependency version checks and hooks registration are correctly wrapped in the instance context.
const nuxt = getNuxtCtx().tryUse() | ||
if (!nuxt) { | ||
logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v4.') | ||
return nuxtCtx.tryUse() | ||
} | ||
return nuxt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update deprecation warning message
The warning message has the same v4/v5 issue as in the useNuxt
function.
- logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v4.')
+ logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v5.')
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const nuxt = getNuxtCtx().tryUse() | |
if (!nuxt) { | |
logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v4.') | |
return nuxtCtx.tryUse() | |
} | |
return nuxt | |
const nuxt = getNuxtCtx().tryUse() | |
if (!nuxt) { | |
logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v5.') | |
return nuxtCtx.tryUse() | |
} | |
return nuxt |
const instance = getNuxtCtx().tryUse() | ||
if (!instance) { | ||
const fallbackInstance = nuxtCtx.tryUse() | ||
if (fallbackInstance) { | ||
logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v4.') | ||
return fallbackInstance | ||
} | ||
|
||
throw new Error('Nuxt instance is unavailable!') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update deprecation warning message
The warning message incorrectly states that the fallback behaviour will be removed in v4, but according to the past review comments, this is planned for v5.
- logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v4.')
+ logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v5.')
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const instance = getNuxtCtx().tryUse() | |
if (!instance) { | |
const fallbackInstance = nuxtCtx.tryUse() | |
if (fallbackInstance) { | |
logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v4.') | |
return fallbackInstance | |
} | |
throw new Error('Nuxt instance is unavailable!') | |
} | |
const instance = getNuxtCtx().tryUse() | |
if (!instance) { | |
const fallbackInstance = nuxtCtx.tryUse() | |
if (fallbackInstance) { | |
logger.warn('Using fallback global Nuxt instance. You may be using a @nuxt/kit composable outside of a Nuxt context, this behavior is deprecated and will be removed in v5.') | |
return fallbackInstance | |
} | |
throw new Error('Nuxt instance is unavailable!') | |
} |
🔗 Linked issue
unblocked by nitrojs/nitro#2983 ❤️
fix nuxt/test-utils#537
fix nuxt/test-utils#1034
resolve nuxt/test-utils#664
fix #22565
📚 Description
This PR allows to create multiple Nuxt instance within a single process.
We were previously setting a Nuxt Singleton with unctx. This means that we can only run a single Nuxt instance in a single process.
By using asyncLocalStorage, we'll be able to track an unique id for each
buildNuxt
andinitNuxt
-> allowing to run these in parallel and alluseNuxt()
within it. It also convertnuxtCtx
to be a getter so it retrieves the correct namespace withASyncLocalStorage
For backward compatibility, we'll keep a fallback global singleton of the first Nuxt instance that is being set. This behavior CAN be removed in V5. We can also add a
.run
on the Nuxt build instance later.next step: merge all nuxt test within a single job