-
Notifications
You must be signed in to change notification settings - Fork 108
feat(runtime)!: change nuxt start timing to beforeAll hook #1516
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
base: main
Are you sure you want to change the base?
Conversation
commit: |
|
Iβve realized there are some points I haven't fully considered yet, so Iβll revert this to a draft for further refinement. My apologies. |
1823a5c to
c2fbe7b
Compare
c2fbe7b to
4a89806
Compare
4a89806 to
575fbba
Compare
7e235ea to
297fb17
Compare
|
@yamachi4416 are you on Discord or Bluesky/Twitter? would you mind sending me a DM (nothing bad!)? |
|
It turns out that referencing |
145cf17 to
e71dfc7
Compare
|
I wish I could have supported |
danielroe
left a comment
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.
what are the downsides of using beforeAll?
(we could, for example, make this the new default)
|
thank you for the review. the main downsides of using if I'm starting to feel that it might be better to make |
|
agreed- I think |
|
thank you for the advice. |
e71dfc7 to
4bf9064
Compare
π WalkthroughWalkthroughAdds a global counter composable, a plugin that provides it, and a route middleware that uses the counter to conditionally navigate. Extends the inject-value plugin to provide and export a second Symbol-based injection. Introduces new components exercising Nuxt core composables and updates an existing component to inject two values. Adds and expands many Vitest tests covering composable/plugin/middleware behavior, mocking of Nuxt core composables and vue-router, and lifecycle handling. Moves runtime entry Nuxt setup into a Vitest beforeAll hook. Estimated code review effortπ― 4 (Complex) | β±οΈ ~60 minutes π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing touches
π§ͺ Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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: 1
π€ Fix all issues with AI agents
In `@examples/app-vitest-full/tests/nuxt/middleware.spec.ts`:
- Line 33: The test description for the spec named "can original nuxt core
composable inside middleware" has a typoβadd the missing verb to read "can use
original nuxt core composable inside middleware". Update the string in the
it(...) call in tests/nuxt/middleware.spec.ts (the test labelled by that
description) to "can use original nuxt core composable inside middleware".
π§Ή Nitpick comments (4)
examples/app-vitest-full/tests/nuxt/mock-vue-router.spec.ts (1)
18-21: Consider removingvi.restoreAllMocks()or verifying expected behavior.
vi.restoreAllMocks()restores spied functions to their original implementations. However, since you're usingvi.mock()(which is hoisted and persistent), callingrestoreAllMocks()afterresetAllMocks()may not behave as intended. Thevi.fn(original)wrappers from lines 11 and 15 should persist through resets.If the intent is just to clear mock call history between tests,
vi.resetAllMocks()alone should suffice.Suggested simplification
beforeEach(() => { vi.resetAllMocks() - vi.restoreAllMocks() })examples/app-vitest-full/tests/nuxt/mock-nuxt-composable-1.spec.ts (3)
15-22: Consider Symbol-keyed properties if needed for complete merging.
Object.getOwnPropertyNamescaptures non-enumerable properties but excludes Symbol-keyed properties. Given the PR objectives mention Symbol-based injection testing (#750,#836), verify this limitation doesn't affect test scenarios. If Symbol keys are needed, consider addingObject.getOwnPropertySymbols:β»οΈ Optional enhancement
function mergeObject<V, T extends Record<string, V>>(target: T, merge: Partial<T>) { return { ...Object.fromEntries( - Object.getOwnPropertyNames(target).map(key => [key, target[key]]), + [ + ...Object.getOwnPropertyNames(target), + ...Object.getOwnPropertySymbols(target), + ].map(key => [key, target[key as keyof T]]), ), ...merge, } as T }
25-28: Clarify the mock reset strategy.Using both
resetAllMocks()andrestoreAllMocks()is redundant here.restoreAllMocks()only affects spies created withvi.spyOn(), notvi.fn()mocks. Since mockNuxtImport usesvi.fn(), consider:
- Use
vi.clearAllMocks()if only clearing call history between tests (preserves mock implementations set in nested beforeEach hooks).- Use
vi.resetAllMocks()alone if resetting implementations is intended.The nested
beforeEachhooks in each describe block re-set implementations anyway, soclearAllMocks()may be more appropriate here.
118-123: Minor inconsistency: spread vs mergeObject.Line 121 uses the spread operator while other mock implementations use
mergeObject. The spread operator only copies enumerable properties, whereasmergeObjecthandles non-enumerable properties. Consider usingmergeObjectfor consistency:β»οΈ Suggested consistency fix
beforeEach(() => { useRouteMock.mockImplementation((...args) => { const original = useRouteOriginal(...args) - return { ...original, path: '/mocked' } + return mergeObject(original, { path: '/mocked' }) }) })
4bf9064 to
bda3496
Compare
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: 1
π€ Fix all issues with AI agents
In `@examples/app-vitest-full/tests/nuxt/mock-nuxt-composable-1.spec.ts`:
- Around line 15-21: mergeObject currently uses
Object.getOwnPropertyNames(target) which omits Symbol keys; update the function
to include symbol-keyed properties by collecting both
Object.getOwnPropertyNames(target) and Object.getOwnPropertySymbols(target) (or
use Reflect.ownKeys/Object.getOwnPropertyDescriptors) when building the entries
from the target parameter in mergeObject, and adjust the type constraint (e.g.,
allow symbol keys via PropertyKey or Record<PropertyKey, V>) so symbol
properties are preserved in the returned object before spreading in merge.
bda3496 to
8396dcc
Compare
π Linked issue
resolves #750
resolves #836
resolves #1185
resolves #1496
β Type of change
π Description
The timing for starting Nuxt has been moved from setupFiles to
beforeAllhook. This change enables mocking of composables that are loaded during Nuxt initialization.Please note that migration is required for certain cases, such as test code that partially mocks Nuxt core composables, or code that mounts components outside of the test suite (which requires Nuxt to be initialized). Modifying the test code is required for these patterns to function.
since
mockNuxtImportnow provides the original composable in its factory function, test code will be updated to mock only the necessary parts using this feature. This approach ensures that Nuxt initialization does not fail even when partially mocking core composables.For cases where components are mounted outside of the test suite, there are options to either move the mounting logic inside the test suite or perform the mount within
beforeAllhook.Reproductions
expected:
eventBus $on EVENT XXX, theXXXvalues match, andapp.vue - onEvent()is logged.