feat: add support for custom source-extensions to watchers and bundlers#4231
Open
seanstrom wants to merge 4 commits intonitrojs:mainfrom
Open
feat: add support for custom source-extensions to watchers and bundlers#4231seanstrom wants to merge 4 commits intonitrojs:mainfrom
seanstrom wants to merge 4 commits intonitrojs:mainfrom
Conversation
|
@seanstrom is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
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:
📝 WalkthroughWalkthroughIntroduces a configurable Unified Source-Extension Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/build/rolldown/dev.ts`:
- Around line 44-45: The root watcher regex only matches "server.<ext>" and
misses "server.node.<ext>"; update the serverEntryRe construction to accept an
optional ".node" segment by changing the pattern to something like
RegExp(String.raw`^server(?:\.node)?\.(?:${sourceExtensionPattern})$`) so both
server.<ext> and server.node.<ext> are matched—update the line that defines
serverEntryRe (which currently uses getSourceExtensionPattern and
sourceExtensionPattern) accordingly.
In `@src/build/rollup/dev.ts`:
- Around line 46-47: The server entry regex (serverEntryRe) currently only
matches "server.<ext>" and misses "server.node.<ext>"; update the RegExp
construction that uses sourceExtensionPattern so it accepts an optional ".node"
segment (e.g. change the pattern used in serverEntryRe to something like
^server(?:\.node)?\.(?:${sourceExtensionPattern})$) so both "server.<ext>" and
"server.node.<ext>" trigger the watcher; adjust the serverEntryRe initialization
that references sourceExtensionPattern accordingly.
In `@src/utils/source-extensions.ts`:
- Around line 15-20: normalizeSourceExtensions currently accepts invalid tokens
(e.g. "." or strings containing glob/path chars) which can produce malformed
scan/regex patterns; update normalizeSourceExtensions to validate each token
after trimming: reject and throw an Error for tokens that are exactly "." or
contain any of the characters used in globs/paths (e.g. '*', '?', '[', ']', '{',
'}', '/', '\\', path separators) or that are empty after trimming, and include
the offending token in the error message so callers know which value failed;
keep existing normalization behavior (prepend "." when missing) for valid tokens
and consider deduplicating the result before returning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ded3dee-bb65-4c1e-93f3-32c8e2afffb8
📒 Files selected for processing (20)
docs/1.docs/50.configuration.mdsrc/build/config.tssrc/build/rolldown/dev.tssrc/build/rollup/dev.tssrc/build/types.tssrc/build/vite/dev.tssrc/build/vite/env.tssrc/build/vite/plugin.tssrc/config/defaults.tssrc/config/resolvers/paths.tssrc/module.tssrc/presets/cloudflare/entry-exports.tssrc/scan.tssrc/types/config.tssrc/utils/source-extensions.tstest/fixture/nitro.config.tstest/fixture/server/routes/api/civet.civettest/tests.tstest/vite/hmr-fixture/api/state-source-extension.civettest/vite/hmr.test.ts
Author
|
@coderabbitai review please |
Contributor
|
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/utils/source-extensions.ts (1)
15-24:⚠️ Potential issue | 🟠 MajorFail fast on invalid extension tokens instead of silently filtering.
normalizeSourceExtensionscurrently drops invalid values ("",".") and still allows path/glob-special characters. That can produce malformed or over-broad glob/regex patterns downstream. Please throw with the offending token.As per coding guidelines: "Prefer explicit errors over silent failures; use warnings for recoverable situations and throw for invalid states" and "Include actionable context in error messages".Proposed hard validation
export function normalizeSourceExtensions(extensions: string[] = []) { return extensions - .map((ext) => ext.trim()) - .filter((ext) => { - const trimmedExt = ext.trim(); - const isEmpty = trimmedExt.length === 0; - const isInvalid = isEmpty || trimmedExt === "."; - return !isInvalid; - }) - .map((ext) => (ext.startsWith(".") ? ext : `.${ext}`)); + .map((rawExt) => { + const trimmedExt = rawExt.trim(); + const normalizedExt = trimmedExt.startsWith(".") + ? trimmedExt + : `.${trimmedExt}`; + const extBody = normalizedExt.slice(1); + + if (!extBody || /[\/\\*?[\]{}(),]/.test(extBody)) { + throw new Error( + `Invalid source extension "${rawExt}". Use a plain extension like ".civet" without path/glob characters.` + ); + } + return normalizedExt; + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/source-extensions.ts` around lines 15 - 24, normalizeSourceExtensions currently silently filters invalid tokens; change it to validate each input and throw a descriptive Error when encountering an offending token. Specifically, in normalizeSourceExtensions iterate over the input extensions and for each token (e.g., variable trimmedExt) check for empty string, a single dot ".", and any path/glob-special characters (such as "/", "\\", "*", "?", "[", "]", "{", "}", "(", ")", "!", "|", etc.); if any of these are present throw an Error that includes the offending token and a short actionable message. If the token is valid, normalize it to start with a dot (ext.startsWith(".") ? ext : `.${ext}`) and return the normalized array.
🧹 Nitpick comments (2)
src/utils/source-extensions.ts (2)
27-59: Prefer options-object second parameters for these exported APIs.
getSourceExtensions,getSourceExtensionPattern, andstripSourceExtensioncurrently rely on positionalbaseExtensions, which is easier to misuse and diverges from repo convention.As per coding guidelines: "For multi-arg functions, use an options object as the second parameter".API-shape refactor (same behavior)
export function getSourceExtensions( { sourceExtensions }: SourceExtensionOptions, - baseExtensions = BASE_SOURCE_EXTENSIONS + options: { baseExtensions?: string[] } = {} ) { + const { baseExtensions = BASE_SOURCE_EXTENSIONS } = options; return [...new Set([...baseExtensions, ...normalizeSourceExtensions(sourceExtensions)])]; } export function getSourceExtensionPattern( { sourceExtensions }: SourceExtensionOptions, - baseExtensions = BASE_SOURCE_EXTENSIONS + options: { baseExtensions?: string[] } = {} ) { - return getSourceExtensions({ sourceExtensions }, baseExtensions) + return getSourceExtensions({ sourceExtensions }, options) .map((ext) => escapeRegExp(ext.slice(1))) .join("|"); } export function stripSourceExtension( id: string, options: SourceExtensionOptions, - baseExtensions = BASE_SOURCE_EXTENSIONS + extra: { baseExtensions?: string[] } = {} ) { - const ext = getSourceExtensions(options, baseExtensions) + const ext = getSourceExtensions(options, extra) .sort((a, b) => b.length - a.length) .find((ext) => id.endsWith(ext)); return ext ? id.slice(0, -ext.length) : id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/source-extensions.ts` around lines 27 - 59, These exported functions use a positional second parameter for baseExtensions which violates the options-object convention; change the signatures of getSourceExtensions, getSourceExtensionPattern, and stripSourceExtension to accept a second options object (e.g. { baseExtensions } = {}) instead of a positional array, defaulting baseExtensions to BASE_SOURCE_EXTENSIONS, update internal callers to read baseExtensions from that options object (and keep normalizeSourceExtensions calls and existing behavior), and also update getScanPattern to continue calling getSourceExtensions(options) unchanged; search for and update all usages of getSourceExtensions(..., baseExtensions), getSourceExtensionPattern(..., baseExtensions), and stripSourceExtension(..., baseExtensions) to pass an options object { baseExtensions } so behavior remains the same while following the options-object API.
6-9: Move internal helper constants to the end of the file.
moduleExtensionsis non-exported/internal and is currently declared before exported members.As per coding guidelines: "Place non-exported/internal helpers at the end of the file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/source-extensions.ts` around lines 6 - 9, The non-exported helper constant moduleExtensions is declared before exported members; move the const moduleExtensions declaration to the end of the file after all exported functions/types so internal helpers follow the coding guideline. Ensure you keep the same const name and values, do not export it, and update any internal references (e.g., where exported functions use moduleExtensions) — if those functions reference it, keep their implementations above but rely on the module-scoped constant defined later (no code changes needed besides reordering).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/utils/source-extensions.ts`:
- Around line 15-24: normalizeSourceExtensions currently silently filters
invalid tokens; change it to validate each input and throw a descriptive Error
when encountering an offending token. Specifically, in normalizeSourceExtensions
iterate over the input extensions and for each token (e.g., variable trimmedExt)
check for empty string, a single dot ".", and any path/glob-special characters
(such as "/", "\\", "*", "?", "[", "]", "{", "}", "(", ")", "!", "|", etc.); if
any of these are present throw an Error that includes the offending token and a
short actionable message. If the token is valid, normalize it to start with a
dot (ext.startsWith(".") ? ext : `.${ext}`) and return the normalized array.
---
Nitpick comments:
In `@src/utils/source-extensions.ts`:
- Around line 27-59: These exported functions use a positional second parameter
for baseExtensions which violates the options-object convention; change the
signatures of getSourceExtensions, getSourceExtensionPattern, and
stripSourceExtension to accept a second options object (e.g. { baseExtensions }
= {}) instead of a positional array, defaulting baseExtensions to
BASE_SOURCE_EXTENSIONS, update internal callers to read baseExtensions from that
options object (and keep normalizeSourceExtensions calls and existing behavior),
and also update getScanPattern to continue calling getSourceExtensions(options)
unchanged; search for and update all usages of getSourceExtensions(...,
baseExtensions), getSourceExtensionPattern(..., baseExtensions), and
stripSourceExtension(..., baseExtensions) to pass an options object {
baseExtensions } so behavior remains the same while following the options-object
API.
- Around line 6-9: The non-exported helper constant moduleExtensions is declared
before exported members; move the const moduleExtensions declaration to the end
of the file after all exported functions/types so internal helpers follow the
coding guideline. Ensure you keep the same const name and values, do not export
it, and update any internal references (e.g., where exported functions use
moduleExtensions) — if those functions reference it, keep their implementations
above but rely on the module-scoped constant defined later (no code changes
needed besides reordering).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c80394e9-c029-45ae-9e53-023af49b5a48
📒 Files selected for processing (1)
src/utils/source-extensions.ts
seanstrom
force-pushed
the
seanstrom/add-custom-source-extension-support
branch
from
bc440c9 to
e680dd4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/vite/hmr.test.ts`:
- Around line 97-144: The test "editing custom source extension API entry" can
exceed the default 5s Vitest timeout because it creates a Vite dev server, waits
for an initial fetch, edits a file and polls for HMR; update the test
declaration to increase the timeout (e.g. to 30_000 ms) by adding a timeout
argument to the test call so the async function that uses createServer,
customServer.listen, and pollResponse has enough time to complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 767043de-6eef-403f-8383-33a74830ac6b
📒 Files selected for processing (20)
docs/1.docs/50.configuration.mdsrc/build/config.tssrc/build/rolldown/dev.tssrc/build/rollup/dev.tssrc/build/types.tssrc/build/vite/dev.tssrc/build/vite/env.tssrc/build/vite/plugin.tssrc/config/defaults.tssrc/config/resolvers/paths.tssrc/module.tssrc/presets/cloudflare/entry-exports.tssrc/scan.tssrc/types/config.tssrc/utils/source-extensions.tstest/fixture/nitro.config.tstest/fixture/server/routes/api/civet.civettest/tests.tstest/vite/hmr-fixture/api/state-source-extension.civettest/vite/hmr.test.ts
✅ Files skipped from review due to trivial changes (6)
- test/fixture/server/routes/api/civet.civet
- test/vite/hmr-fixture/api/state-source-extension.civet
- test/fixture/nitro.config.ts
- docs/1.docs/50.configuration.md
- src/build/config.ts
- src/utils/source-extensions.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/module.ts
- src/presets/cloudflare/entry-exports.ts
- src/build/vite/plugin.ts
- src/build/vite/dev.ts
- src/build/vite/env.ts
- src/types/config.ts
- src/build/types.ts
Comment on lines
+97
to
+144
| test("editing custom source extension API entry", async () => { | ||
| const customAPI = openFileForEditing(join(rootDir, "api/state-source-extension.civet")); | ||
| const customMessages: any[] = []; | ||
| const customServer = await createServer({ | ||
| root: rootDir, | ||
| configFile: false, | ||
| plugins: [ | ||
| { | ||
| name: "test:source-extensions", | ||
| enforce: "pre", | ||
| transform(code: any, id: any) { | ||
| if (id.endsWith(".civet")) { | ||
| return { code, map: null }; | ||
| } | ||
| }, | ||
| }, | ||
| nitro({ serverDir: "./", sourceExtensions: [".civet"] }), | ||
| ], | ||
| }); | ||
| const originalSend = customServer.ws.send.bind(customServer.ws); | ||
| customServer.ws.send = function (payload: any) { | ||
| customMessages.push(payload); | ||
| return originalSend(payload); | ||
| }; | ||
|
|
||
| try { | ||
| await customServer.listen("0" as unknown as number); | ||
| const addr = customServer.httpServer?.address() as { | ||
| port: number; | ||
| address: string; | ||
| family: string; | ||
| }; | ||
| const customServerURL = `http://${addr.family === "IPv6" ? `[${addr.address}]` : addr.address}:${addr.port}`; | ||
| const initialResponse = await fetch(`${customServerURL}/api/state-source-extension`).then( | ||
| (r) => r.text() | ||
| ); | ||
| expect(initialResponse).toContain('"state":1'); | ||
|
|
||
| customAPI.update((content) => | ||
| content.replace("({ state })", '({ state: state + " (modified)" })') | ||
| ); | ||
| await pollResponse(`${customServerURL}/api/state-source-extension`, /modified/); | ||
| expect(customMessages).toMatchObject([{ type: "full-reload" }]); | ||
| } finally { | ||
| customAPI.restore(); | ||
| await customServer.close(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Add an explicit timeout — default 5 s is insufficient for server creation + HMR polling.
The test creates a brand-new Vite dev server (which can take several seconds in CI), waits for an initial fetch, edits a file, and then calls pollResponse (up to 5 s internally). All of this must complete within Vitest's default 5000 ms test timeout, making it nearly certain to time out in any moderately loaded CI environment. The existing beforeAll uses 30_000 ms for the same reason.
⏱️ Proposed fix
- test("editing custom source extension API entry", async () => {
+ test("editing custom source extension API entry", { timeout: 30_000 }, async () => {📝 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.
Suggested change
| test("editing custom source extension API entry", async () => { | |
| const customAPI = openFileForEditing(join(rootDir, "api/state-source-extension.civet")); | |
| const customMessages: any[] = []; | |
| const customServer = await createServer({ | |
| root: rootDir, | |
| configFile: false, | |
| plugins: [ | |
| { | |
| name: "test:source-extensions", | |
| enforce: "pre", | |
| transform(code: any, id: any) { | |
| if (id.endsWith(".civet")) { | |
| return { code, map: null }; | |
| } | |
| }, | |
| }, | |
| nitro({ serverDir: "./", sourceExtensions: [".civet"] }), | |
| ], | |
| }); | |
| const originalSend = customServer.ws.send.bind(customServer.ws); | |
| customServer.ws.send = function (payload: any) { | |
| customMessages.push(payload); | |
| return originalSend(payload); | |
| }; | |
| try { | |
| await customServer.listen("0" as unknown as number); | |
| const addr = customServer.httpServer?.address() as { | |
| port: number; | |
| address: string; | |
| family: string; | |
| }; | |
| const customServerURL = `http://${addr.family === "IPv6" ? `[${addr.address}]` : addr.address}:${addr.port}`; | |
| const initialResponse = await fetch(`${customServerURL}/api/state-source-extension`).then( | |
| (r) => r.text() | |
| ); | |
| expect(initialResponse).toContain('"state":1'); | |
| customAPI.update((content) => | |
| content.replace("({ state })", '({ state: state + " (modified)" })') | |
| ); | |
| await pollResponse(`${customServerURL}/api/state-source-extension`, /modified/); | |
| expect(customMessages).toMatchObject([{ type: "full-reload" }]); | |
| } finally { | |
| customAPI.restore(); | |
| await customServer.close(); | |
| } | |
| }); | |
| test("editing custom source extension API entry", { timeout: 30_000 }, async () => { | |
| const customAPI = openFileForEditing(join(rootDir, "api/state-source-extension.civet")); | |
| const customMessages: any[] = []; | |
| const customServer = await createServer({ | |
| root: rootDir, | |
| configFile: false, | |
| plugins: [ | |
| { | |
| name: "test:source-extensions", | |
| enforce: "pre", | |
| transform(code: any, id: any) { | |
| if (id.endsWith(".civet")) { | |
| return { code, map: null }; | |
| } | |
| }, | |
| }, | |
| nitro({ serverDir: "./", sourceExtensions: [".civet"] }), | |
| ], | |
| }); | |
| const originalSend = customServer.ws.send.bind(customServer.ws); | |
| customServer.ws.send = function (payload: any) { | |
| customMessages.push(payload); | |
| return originalSend(payload); | |
| }; | |
| try { | |
| await customServer.listen("0" as unknown as number); | |
| const addr = customServer.httpServer?.address() as { | |
| port: number; | |
| address: string; | |
| family: string; | |
| }; | |
| const customServerURL = `http://${addr.family === "IPv6" ? `[${addr.address}]` : addr.address}:${addr.port}`; | |
| const initialResponse = await fetch(`${customServerURL}/api/state-source-extension`).then( | |
| (r) => r.text() | |
| ); | |
| expect(initialResponse).toContain('"state":1'); | |
| customAPI.update((content) => | |
| content.replace("({ state })", '({ state: state + " (modified)" })') | |
| ); | |
| await pollResponse(`${customServerURL}/api/state-source-extension`, /modified/); | |
| expect(customMessages).toMatchObject([{ type: "full-reload" }]); | |
| } finally { | |
| customAPI.restore(); | |
| await customServer.close(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/vite/hmr.test.ts` around lines 97 - 144, The test "editing custom source
extension API entry" can exceed the default 5s Vitest timeout because it creates
a Vite dev server, waits for an initial fetch, edits a file and polls for HMR;
update the test declaration to increase the timeout (e.g. to 30_000 ms) by
adding a timeout argument to the test call so the async function that uses
createServer, customServer.listen, and pollResponse has enough time to complete.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Related Discussions
❓ Type of change
📚 Description
.civetfile extension.📝 Checklist