-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: url rewrites #5029
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
feat: url rewrites #5029
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 6m 3s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 18s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-08-27 21:07:14 UTC
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds URL-centric location handling across history and routers: locations now carry a URL object and fullPath; parsing uses full href; hash/browser histories generate absolute URLs; link components use location.url.href; router introduces rewrite hooks (toURL/fromURL) and derives reload href from location.url.href. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant B as Browser
participant H as History (parseHref)
participant R as Router (parse/build)
participant RW as Rewrite Hooks
participant L as Link Components
Note over B,H: Initial parse on load/navigation
B->>H: parseHref(href)
H->>H: new URL(href | fallback)
H-->>R: HistoryLocation { url, pathname, search, hash, state }
Note over R,RW: Optional URL rewrite (toURL)
R->>RW: toURL({ url })
alt toURL returns URL/string
RW-->>R: rewritten URL
R->>R: derive pathname/search/hash/fullPath from rewritten URL
else no rewrite
RW-->>R: undefined (no change)
end
R-->>R: ParsedLocation { url, fullPath, href=fullPath, ... }
Note over R,RW: Build/navigation with optional rewrite (fromURL)
R->>R: build next URL from location intent
R->>RW: fromURL({ url })
alt fromURL returns URL/string
RW-->>R: rewritten URL
else
RW-->>R: undefined (no change)
end
R-->>H: absolute href via URL.href
H-->>B: push/replaceState(href)
Note over L: Link rendering
R-->>L: next/masked ParsedLocation with url
L->>L: href = masked?.url.href : next.url.href
sequenceDiagram
autonumber
participant U as User
participant R as Router.navigate
participant B as Browser
U->>R: navigate({ reloadDocument: true, location without href })
R->>R: derive href from location.url.href
R-->>B: window.location.assign(href)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
🧹 Nitpick comments (1)
packages/history/src/index.ts (1)
299-302: Consider documenting the parameter type change frompathtohref.While the implementation preserves backward compatibility by using the same pass-through behavior, the parameter name change from
pathtohrefin the defaultcreateHreffunction could be confusing. Consider adding a comment or documentation to clarify this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/history/src/index.ts(4 hunks)packages/react-router/src/link.tsx(1 hunks)packages/router-core/src/location.ts(1 hunks)packages/router-core/src/router.ts(4 hunks)packages/solid-router/src/link.tsx(1 hunks)
🔇 Additional comments (11)
packages/router-core/src/location.ts (2)
27-32: LGTM! Good deprecation strategy forpathnameproperty.The deprecation notice for
pathnameprovides clear guidance to uselocation.url.pathnameinstead, maintaining backward compatibility while directing users toward the new URL-centric approach.
33-57: LGTM! Well-structured location properties.The new properties (
search,searchStr,state,hash,maskedLocation,unmaskOnReload) are properly documented with JSDoc comments that clearly explain their purpose.packages/solid-router/src/link.tsx (1)
391-396: LGTM! Consistent URL-centric href resolution.The change to use
location.url.hrefdirectly instead ofrouter.history.createHref()aligns well with the PR's goal of URL-centric location handling and maintains consistency with the React implementation.packages/react-router/src/link.tsx (1)
351-356: LGTM! URL-centric href resolution implemented correctly.The change to use
location.url.hrefdirectly instead ofrouter.history.createHref()is consistent with the overall URL-centric approach and matches the implementation in the Solid router.packages/history/src/index.ts (3)
46-52: LGTM! Clean addition of URL property to ParsedPath interface.The addition of the
url: URLproperty to theParsedPathinterface is well-placed and properly documented by being included as the first property.
560-565: LGTM! Proper absolute URL construction for hash history.The change to use
new URL()to construct absolute URLs with the origin is correct and ensures consistent URL handling across different history types.
619-652: Solid URL parsing implementation with good fallback handling.The
parseHreffunction properly handles both absolute and relative URLs with a sensible fallback tohttp://localhostfor relative paths. The URL object is correctly added to the returnedHistoryLocation.packages/router-core/src/router.ts (4)
439-454: LGTM! Well-structured URL rewrite hooks.The new
rewriteoption withfromURLandtoURLhooks is properly documented and provides a clean API for URL transformations. The JSDoc comments clearly explain the purpose and behavior of each hook.
1053-1087: LGTM! Clean implementation of URL rewriting in parseLocation.The URL rewriting logic is properly integrated:
toURLhook is applied before extracting pathname, search, and hash- URL properties are correctly derived from the transformed URL
fullPathis properly calculated by removing the origin- The TODO comment correctly notes the future change for
hrefin v2
1866-1870: LGTM! Correct use of location.url.href for reload navigation.The change to use
location.url.hrefwhenreloadDocumentis true and no explicithrefis provided ensures the full URL is used for the reload, which is correct behavior.
1642-1669: Verify URL rewriting sequence inbuildLocationI didn’t find any existing tests or documentation that cover how your
rewrite.toURLhook interacts withhistory.createHref. Please manually confirm that invokingcreateHrefafter applying the rewrite hook doesn’t reverse or conflict with the URL modifications your hook produces.• File:
packages/router-core/src/router.ts
• Lines: 1642–1669
Add a few more setup configurations to measure the number of `__store` updates: - with caching (`staleTime > 0`) - when preloading multiple times the same route (simulates hovering a list of links to the same page w/ different params) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
4f2fae5 to
2ea6460
Compare
Summary by CodeRabbit