Add Nuxt module for frictionless spa setup#160
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Nuxt 3 SPA module that registers a client-only oidc-spa Vite plugin, exposes the module via a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
49-87:⚠️ Potential issue | 🔴 CriticalMove
@nuxt/kitfrom optional peerDependencies to dependencies.The
src/nuxt-spa/index.tsmodule imports@nuxt/kitdirectly at runtime (defineNuxtModule,addVitePlugin), making it a required dependency, not optional. Per Nuxt's official Kit guide,@nuxt/kitmust be independencies, notpeerDependencies. Placing it only as an optional peer will cause Nuxt consumers (especially pnpm users) to encounter "Cannot find module" errors unless they manually add@nuxt/kitto their own dependencies—defeating the purpose of module encapsulation.Move
@nuxt/kit(and@nuxt/schema) frompeerDependenciesintodependencies. Remove the optional metadata entries for both. Also ensure versions match or exceed the consumer'snuxtversion (currently^4.3.1is appropriate).Also applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 49 - 87, The package.json currently lists `@nuxt/kit` (and `@nuxt/schema`) as optional peerDependencies but src/nuxt-spa/index.ts imports defineNuxtModule and addVitePlugin at runtime, so move "@nuxt/kit" and "@nuxt/schema" from "peerDependencies" into "dependencies" (using versions at or above the project's nuxt range, e.g. ^4.3.1), remove their entries from "peerDependenciesMeta", and delete them from the peerDependencies block; keep other peers unchanged so consumers no longer get "Cannot find module" errors when importing src/nuxt-spa/index.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package.json`:
- Around line 49-87: The package.json currently lists `@nuxt/kit` (and
`@nuxt/schema`) as optional peerDependencies but src/nuxt-spa/index.ts imports
defineNuxtModule and addVitePlugin at runtime, so move "@nuxt/kit" and
"@nuxt/schema" from "peerDependencies" into "dependencies" (using versions at or
above the project's nuxt range, e.g. ^4.3.1), remove their entries from
"peerDependenciesMeta", and delete them from the peerDependencies block; keep
other peers unchanged so consumers no longer get "Cannot find module" errors
when importing src/nuxt-spa/index.ts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
package.jsonscripts/build.tssrc/nuxt-spa/index.ts
garronej
left a comment
garronej
left a comment
There was a problem hiding this comment.
Thanks a lot! LGTM beside the listing of the peer deps
| "@nuxt/kit": "^4.3.1", | ||
| "@nuxt/schema": "^4.3.1", |
There was a problem hiding this comment.
Don't we want that as optional peer dependency?
There was a problem hiding this comment.
I moved them away from peerDependencies based on a (mis)reading of the Nuxt docs + the CodeRabbit review. After digging in a bit more, it’s clear these should be peerDependencies. Sorry about that. Hopefully now correctly done.
There was a problem hiding this comment.
Actually, it doesn’t really matter.
If users are working with Nuxt, the dependency will already be available in their project.
We declare it because some package managers perform advanced checks and will emit warnings if a library imports a dependency that isn’t listed as either a dependency or a peerDependency. So this is mostly ceremonial.
Looking at your PR, it seems that only @nuxt/kit is explicitly imported.
Therefore, it should be the only peer dependency we declare.
@nuxt/schema can remain in devDependencies for local development purposes.
There was a problem hiding this comment.
Removed @nuxt/schema from the peer dependencies, but kept @nuxt/kit and @nuxt/schema at the same version in package.json, since Nuxt strongly recommends this for authoring Nuxt modules.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package.json (1)
53-54: Consider constraining the peer dependency version range for@nuxt/kit/@nuxt/schema.The wildcard
"*"allows any version, including Nuxt 3-era@nuxt/kit(e.g.3.x). The module was authored and tested against Nuxt 4; binding to">=4.0.0"would prevent subtle breakage if a consumer happens to have a Nuxt 3 workspace installed alongside.♻️ Suggested version constraint
- "@nuxt/kit": "*", - "@nuxt/schema": "*", + "@nuxt/kit": ">=4.0.0", + "@nuxt/schema": ">=4.0.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 53 - 54, Change the peerDependency entries that currently use "*" for `@nuxt/kit` and `@nuxt/schema` to a constrained Nuxt 4 range (e.g. ">=4.0.0") so the package will not be satisfied by Nuxt 3 artifacts; update the package.json peerDependencies for the symbols "@nuxt/kit" and "@nuxt/schema" to use the new version range (or a stricter range like ">=4.0.0 <5.0.0") and run a quick install/test to confirm no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Around line 53-54: Change the peerDependency entries that currently use "*"
for `@nuxt/kit` and `@nuxt/schema` to a constrained Nuxt 4 range (e.g. ">=4.0.0") so
the package will not be satisfied by Nuxt 3 artifacts; update the package.json
peerDependencies for the symbols "@nuxt/kit" and "@nuxt/schema" to use the new
version range (or a stricter range like ">=4.0.0 <5.0.0") and run a quick
install/test to confirm no regressions.
| "@nuxt/kit": "^4.3.1", | ||
| "@nuxt/schema": "^4.3.1", |
There was a problem hiding this comment.
Actually, it doesn’t really matter.
If users are working with Nuxt, the dependency will already be available in their project.
We declare it because some package managers perform advanced checks and will emit warnings if a library imports a dependency that isn’t listed as either a dependency or a peerDependency. So this is mostly ceremonial.
Looking at your PR, it seems that only @nuxt/kit is explicitly imported.
Therefore, it should be the only peer dependency we declare.
@nuxt/schema can remain in devDependencies for local development purposes.
| import { defineNuxtModule, addVitePlugin } from "@nuxt/kit"; | ||
| import { oidcSpa, type OidcSpaVitePluginParams } from "../vite-plugin"; | ||
|
|
||
| export default defineNuxtModule<OidcSpaVitePluginParams>().with({ |
There was a problem hiding this comment.
Do we want an export default here?
In the future we might want to export the actual adapter for vue from this module.
In general I prefer named exports. But if you tell me that having a named export for this will break expectations in a major way, I trust your jugement.
There was a problem hiding this comment.
Nuxt expects the module entrypoint (the file resolved from modules: ["…"] in nuxt.config.ts) to expose the module as a default export. That’s what the Nuxt docs/examples use (export default defineNuxtModule(...)), and it’s what makes this work:
// nuxt.config.ts
export default defineNuxtConfig({
modules: ["oidc-spa/nuxt-spa"], // Nuxt loads the entry and uses its default export
});If we switch the entry file to only named exports, the string-based resolution is likely to break / become nonstandard (users would have to import the module manually).
We can still satisfy the preference for named exports by exporting both if wanted:
export const oidcSpaNuxtModule = defineNuxtModule(...);
export default oidcSpaNuxtModule;There was a problem hiding this comment.
Exporting both now.
|
Hey @DPHonys, So sorry for the delayed review. Merging now, looks solid! |
|
I triggered the release of a new version: 10.1.0 |
2 participants
Description
This PR adds a dedicated Nuxt module for
oidc-spa, building on top of the existing Vite plugin support.Problem
While the Vite plugin already handles Nuxt 3 and 4 projects, users still had to manually wire up
oidc-spathroughaddVitePluginin their project modules - an unusual pattern for a Nuxt ecosystem where modules are the standard integration point. There was no idiomatic, first-class way to configureoidc-spainnuxt.config.ts.Solution
Added a new
oidc-spa/nuxt-spaexport that provides a proper Nuxt module built with@nuxt/kit:defineNuxtModulewith config keyoidcSpa, so configuration lives innuxt.config.tsunder{ oidcSpa: { ... } }ssr: falseat module setup time and throws a clear error if SSR is not disabledaddVitePlugin(..., { client: true, server: false })oidc-spa/nuxt-spa) and excluded from the CJS build, as Nuxt modules are ESM-nativeTesting
Tested with Nuxt 4 in SPA mode (
ssr: false) and verified thatoidcEarlyInit()is called correctly via the Vite plugin and that the module configuration flows through properly fromnuxt.config.ts.Usage
Summary by CodeRabbit
New Features
Dependencies