-
Notifications
You must be signed in to change notification settings - Fork 26.4k
docs: add routing best practices prompt rules #62229
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
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@kstepien3 thanks for the proposed change. Could you please provide more info/examples on the current (without the prompt) vs new (with the prompt) generated code and which LLM model did you use? |
@AndrewKushnir sure Perplexity search/best -
|
Webstorm Claude 4 sonnet - Chat instructions -
|
Webstorm Claude 4 sonnet - Chat instructions -
|
I see webstorm has the worst results. Perhaps because the instructions were entered in the settings instead of directly in the chat |
|
||
## Routing | ||
- Use `provideRouter()` with standalone components instead of RouterModule.forRoot() | ||
- Implement lazy loading using loadComponent for feature routes and standalone components |
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 would not limit to features routes as a common limitation: depending on the pap, not everyone lands on the 'home/default' page so it should be lazy loaded too. I'd just state to lazy load by default.
- Use functional route guards (`CanActivateFn`, `CanDeactivateFn`) instead of class-based guards | ||
- Configure preloading strategies: | ||
- Use NoPreloading as default (conserves bandwidth) | ||
- Implement custom strategies for critical routes only |
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'd await the 'prompt user' (^^) to explain what's a critical route. In my opinion, we can't rely on the AI to choose if it's critical or not. Because it's not only based on the feature but the target too: some applications are mainly used in a low network situation, where specific strategy will not be related ot the features themselves;
- Structure routes hierarchically with clear parent-child relationships | ||
- Keep your routing configuration clean and maintainable by separating routes into their own files, especially for different features. | ||
- Define wildcard routes (`**`) for 404 handling when needed | ||
- Use route resolvers to pre-fetch critical data before component activation |
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.
same issue about 'critical'. The AI might end up using route resolvers everywhere as seeing related data as required for the component. Using a resolver is a strategy you need to explain, i would not include it
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
AI best practices without
routing
instructions.What is the new behavior?
Add new section
Routing
and add best practices there.Does this PR introduce a breaking change?
Other information