feat(react-router): add context option to route
#2104
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 82e7a90. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
|
|
Lets not force push this one if multiple of us are working on it. Merge only! |
packages/react-router/src/route.ts
Outdated
| >, | ||
| ) => any) | ||
|
|
||
| routeContext?: Constrain< |
There was a problem hiding this comment.
Limitations of default type parameters and constraints leads me to use this pattern which I've made into a utility type. Long story short, its easy to get stale types which are the default types/constraints and not the inferred types.
Instead Constrain is a conditional type which checks if a type is assignable to another type, if it is then all good, we can infer the type, otherwise we use the target type to force a type check error. This always gives us the up to date types from inference as the conditional type is always ran
| TLoaderDeps extends Record<string, any> = {}, | ||
| TLoaderDataReturn = {}, | ||
| TParentAllContext = {}, | ||
| TRouteContextFn = AnyContext, |
There was a problem hiding this comment.
I'm trying out patterns to remove unnecessary type parameters. Usually we require 2 type parameters per function because we need to check the return type is never. If we infer the whole function, we don't need two type paramters
|
@chorobin I hope this doesn't screw anything up, but I removed Now it's just |
|
Fudge. We might need to keep it, but only as a runtime thing. I don't want people relying on it as a public API, but I do need it to build up merges correctly... |
tannerlinsley
force-pushed
the
route-context
branch
from
c4849d9 to
ecf24b5
Compare
No problem from my side on this. It probably would be confusing to keep it |
|
@SeanCassiere @chorobin @schiller-manuel we good here? |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
“Go, Go, Go”
Tanner Linsley
TanStack LLC
…On Aug 16, 2024 at 5:45 PM -0600, Sean Cassiere ***@***.***>, wrote:
@SeanCassiere commented on this pull request.
@tannerlinsley could the defaultTransformer issue from #2113 also be pushed with this? It's just a matter of moving the transformer from Start into Router.
Other than that, it's just the typing of the arguments of the routeContext and beforeLoad functions. I can make these changes later. Just need the go-ahead.
In packages/react-router/src/router.ts:
> @@ -1180,6 +1203,38 @@ export class Router<
// And also update the searchError if there is one
match.searchError = searchError
+ const parentMatchId = parentMatch?.id
+
+ const parentContext = !parentMatchId
+ ? ((this.options.context as any) ?? {})
+ : (parentMatch.context ?? this.options.context ?? {})
+
+ match.context = {
+ ...parentContext,
+ ...match.__routeContext,
+ ...match.__beforeLoadContext,
+ }
+
+ // Update the match's context
+ const contextFnContext = {
Typing this object with RouteContextOptions.
In packages/react-router/src/router.ts:
> }))
- const { search, params, routeContext, cause } =
+ const { search, params, context, cause } =
this.getMatch(matchId)!
const beforeLoadFnContext = {
Typing this with BeforeLoadContextOptions.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
* chore: type the objects used for `routeContext` and `beforeLoad` * refactor: move the defaultTransformer from start to react-router * test: add tests to the defaultTransformer
| navigate: (opts: any) => | ||
| this.navigate({ ...opts, _fromLocation: next }), | ||
| buildLocation: this.buildLocation, | ||
| cause: match.cause, |
There was a problem hiding this comment.
@tannerlinsley not sure if this was intentionally omitted or not, but cause, abortController, and preload, were not present on this object before but were typed on the RouteContextOptions type.
There was a problem hiding this comment.
Yeah, I made it the same as beforeLoad in this regard. If the types are wrong here they can be removed
There was a problem hiding this comment.
could we take this chance to add previous and next location (in parsed form) as params to all of those functions?
There was a problem hiding this comment.
@schiller-manuel I'd push for that kind of change to go separate from this PR.
A change of this nature would require more sweeping changes to make sure those values are present in the loadMatches function, whilst not really contributing to the task of adding this context function to the route definition and wiring it up.
Once this goes in, if we are adding in the previous and next location options into the shared definition used for context, beforeLoad, and the loader, it'd certainly be more straight-forward.
SeanCassiere
changed the title
context option to route
# Conflicts: # pnpm-lock.yaml
|
@tannerlinsley Thank you for the work on the library. I thought this would be an appropriate place to ask some questions, since you just closed this PR very recently.
Update: Tested preventing inheritance of context state using Thank you and have a good day! |
|
If anything is breaking, please create an issue for it. These closed PRs don't get much visibility in our inboxes 😅 |
5 participants
This PR adds
contextto each route