feat(router-generator): param name syntax check and warn #6472
+274
−0
Conversation
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
Contributor
📝 WalkthroughWalkthroughAdds runtime route-parameter validation to the router generator: a new validator module extracts and checks param names, logs warnings for invalid names, and is invoked during route-node processing. Tests, fixtures, and snapshots for invalid/valid param scenarios were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Generator as Generator (route-node processor)
participant FS as FileSystem (exists check)
participant Validator as validateRouteParams
participant Logger as Logger
Generator->>FS: check file exists for node
alt file exists
Generator->>Validator: validateRouteParams(routePath, filePath, logger)
Validator-->>Logger: warn(...) for each invalid param (if any)
Generator->>Generator: update route cache entry
else file missing
Generator-->>Logger: warn about missing file / skip
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit a517ae5
☁️ Nx Cloud last updated this comment at |
![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: 1
🤖 Fix all issues with AI agents
In `@packages/router-generator/src/validate-route-params.ts`:
- Around line 55-58: The variable `match` is implicitly any; explicitly type it
as the return type of RegExp.exec to satisfy strict TypeScript: declare `match`
as `RegExpExecArray | null` (e.g., `let match: RegExpExecArray | null = null`)
before the loop that uses `bracePattern.exec(segment)`, so the while loop `while
((match = bracePattern.exec(segment)) !== null)` remains valid; update the
declaration near `bracePattern` and `segment` in validate-route-params to use
this explicit type.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-generator/tests/validate-route-params.test.ts`:
- Around line 7-33: The console.warn spy (warnSpy) can leak if generator.run()
throws; wrap the test's async work (creating Generator, awaiting
generator.run(), and assertions) in a try/finally and call warnSpy.mockRestore()
in the finally block so the spy is always restored; locate the warnSpy
declaration and the await generator.run() call in this test and move the
existing assertions into the try, leaving only warnSpy.mockRestore() inside
finally to guarantee restoration even on failure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
We used to extract all route params with regular expressions. This would intrinsically enforce a very strict format for the parameter names:
[a-zA-Z$][a-zA-Z0-9$]*, i.e. a valid javascript variable name.When we rewrote the matcher (#5722), regular params were not matched by a regex anymore (
$param) but more complex param syntaxes were still (e.g.prefix{-$optional}suffix). So we were only half enforcing the parameter name, which is not ideal.Now we would like to move completely off of regexes (#6470) and we should probably enforce some limitations on parameter names, or we risk preventing future evolutions of the syntax if parameter names are too lax, and users start using characters we would eventually like to assign special meaning to.
Solution
This PR proposes we simply add a warning in the console during router generation.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.