-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Data Strategy Configuration #11098
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
Merged
Merged
Data Strategy Configuration #11098
Changes from 48 commits
Commits
Show all changes
56 commits
Select commit
Hold shift + click to select a range
8e2e259
wip
jacob-ebey 6bbc243
2 hours...... 😮💨
jacob-ebey 4c21cf9
add static handler paths
jacob-ebey 5ecb636
added some initial tests
jacob-ebey 2ca1506
fetchers are always called on their own
jacob-ebey 48d23f1
remove unused lines
jacob-ebey 66c8b7d
add a few more tests
jacob-ebey 562cd60
feat: initial support for lazy
jacob-ebey d6707a7
chore: add ssr single fetch example
jacob-ebey 32b4ad6
update fixture
jacob-ebey 42530e0
await lazy routes in example
jacob-ebey 9225980
callLoaderOrAction -> callLoaderOrActionImplementation
brophdawg11 20230d5
filter matches in fixture
jacob-ebey 7e3e660
Merge branch 'dev' into data_strategy
jacob-ebey ef927da
chore: rename to unstable_dataStrategy
jacob-ebey fae7726
chore: fix lint
jacob-ebey e960cf1
chore: update test after rename
jacob-ebey af7fbcd
Minor refactors
brophdawg11 4d42692
Merge branch 'dev' into data_strategy
brophdawg11 719defb
Remove excess callDataStratgy fn
brophdawg11 109f464
Add more unit tests
brophdawg11 359a714
Add decision doc
brophdawg11 0bba762
Rework internals to support HandlerResult and match.handler()
brophdawg11 57333e0
Switch to match.handler API
brophdawg11 fad8182
Bump bundle
brophdawg11 f49bb0c
Update decision doc
brophdawg11 6bb2c92
Remove match.route promise implementation in favor of handler
brophdawg11 53eb1a2
Refactor SuccessResult/ErrorResult to hold the raw response
brophdawg11 64a420e
Convert RedirectResult to use raw response, code cleanup
brophdawg11 c670e77
Bump bundle
brophdawg11 4181202
Refactor to new loadRoute/handler API
brophdawg11 31e619e
Don't export HandlerResult
brophdawg11 1a60803
Bump bundle
brophdawg11 dfdc2ce
Allow loaders/actions to be a boolean
brophdawg11 a088889
Merge branch 'dev' into data_strategy
brophdawg11 acfea93
Add loadRouteIds and bikeshed_load functionality
brophdawg11 bf37bae
Bump bundle
brophdawg11 a8bedaa
Move dataStrategys test to their own file
brophdawg11 3ba3024
Handle falsy errors correctly
brophdawg11 2272fa7
Rename bikeshed_loaderRoutes->resolve/bikeshed_load->shouldLoad
brophdawg11 5bedc16
Add v7_skipActionErrorRevalidation fiuture flag
brophdawg11 cbcd94b
Proxy along ErrorResponse status codes to ErrorResult
brophdawg11 0c7e98b
Bump bundle
brophdawg11 de419c3
Updates
brophdawg11 885bd5c
Mark DecodedResponse exports as unstable
brophdawg11 0141b5e
Merge branch 'dev' into data_strategy
brophdawg11 432fcb2
Remove DecodedResponse and use HandlerResult
brophdawg11 7110845
Merge branch 'dev' into data_strategy
brophdawg11 921ace2
Apply suggestions from code review
brophdawg11 ec2ae97
Merge branch 'dev' into data_strategy
brophdawg11 c72d536
Updates from final review
brophdawg11 aa19ad4
Add skipLoaderErrorBubbling and final tests
brophdawg11 496788e
Fix lint issue
brophdawg11 a68dbaa
Revert "Fix lint issue"
brophdawg11 e516247
Revert "Revert "Fix lint issue"" to trigger CI?
brophdawg11 2a8dc42
Bump buindle
brophdawg11 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| "@remix-run/router": minor | ||
| --- | ||
|
|
||
| Add a new `unstable_dataStrategy` configuration option | ||
|
|
||
| - This option allows Data Router applications to take control over the approach for executing route loaders and actions | ||
| - The default implementation is today's behavior, to fetch all loaders in parallel, but this option allows users to implement more advanced data flows including Remix single-fetch, middleware/context APIs, automatic loader caching, and more\ |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| "@remix-run/router": minor | ||
| --- | ||
|
|
||
| Add a new `unstable_skipActionRevalidation` future flag | ||
|
|
||
| - Currently, active loaders revalidate after any axction, regardless of the result | ||
| - With this flag enabled, actions that return/throw a 4xx/5xx response status will no longer automatically revalidate | ||
| - This should reduce load on your server since it's rare that a 4xx/5xx should actually mutate any data | ||
| - If you need to revalidate after a 4xx/5xx result with this flag enabled, you can still do that via returning `true` from `shouldRevalidate` | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| .DS_Store | ||
| npm-debug.log | ||
|
|
||
| /docs/api/ | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,359 @@ | ||
| # Data Strategy | ||
|
|
||
| Date: 2024-01-31 | ||
|
|
||
| Status: accepted | ||
|
|
||
| ## Context | ||
|
|
||
| In order to implement "Single Fetch" in Remix ([Issue][single-fetch-issue], [RFC][single-fetch-rfc]), we need to expose some level of control over the internal data fetching behaviors of the `@remix-run/router`. This way, while React Router will run loaders in parallel by default, Remix can opt-into making a single fetch call to the server for all loaders. | ||
|
|
||
| ## Decisions | ||
|
|
||
| ### `dataStrategy` | ||
|
|
||
| To achieve the above, we propose to add an optional `dataStrategy` config which can be passed in by the application. The idea is that `dataStrategy` will accept an array of `matches` to load and will return a parallel array of results for those matches. | ||
|
|
||
| ```js | ||
| function dataStrategy(arg: DataStrategyFunctionArgs): DataResult[]; | ||
|
|
||
| interface DataStrategyFunctionArgs<Context = any> | ||
| extends DataFunctionArgs<Context> { | ||
| matches: AgnosticDataStrategyMatch[]; | ||
| } | ||
|
|
||
| interface DataFunctionArgs<Context> { | ||
| request: Request; | ||
| params: Params; | ||
| context?: Context; | ||
| } | ||
| ``` | ||
|
|
||
| There's a [comment][responsibilities-comment] here from Jacob which does a good job of outlining the current responsibilities, but basically React Router in it's current state handles 4 aspects when it comes to executing loaders for a given URL - `dataStrategy` is largely intended to handle step 3: | ||
|
|
||
| 1. Match routes for URL | ||
| 2. Determine what routes to load (via `shouldRevalidate`) | ||
| 3. Call `loader` functions in parallel | ||
| 4. Decode Responses | ||
|
|
||
| ### Inputs | ||
|
|
||
| The primary input is `matches`, since the user needs to know what routes match and eed to have loaders executed. We also wanted to provide a way for the user to call the "default" internal behavior so they could easily change from parallel to sequential without having to re-invent the wheel and manually call loaders, decode responses, etc. The first idea for this API was to pass a `defaultStrategy(match)` parameter so they could call that per-match: | ||
|
|
||
| ```js | ||
| function dataStrategy({ matches }) { | ||
| // Call in parallel | ||
| return Promise.all(matches.map(m => defaultStrategy((m)))); | ||
|
|
||
| // Call sequentially | ||
| let results = [] | ||
| for (let match of matches) { | ||
| results.push(await defaultStrategy(match)) | ||
| } | ||
| return results; | ||
| } | ||
| ``` | ||
|
|
||
| ⚠️ `defaultStrategy` was eliminated in favor of `match.handler`. | ||
|
|
||
| We also originally intended to expose a `type: 'loader' | 'action`' field as a way to presumably let them call `match.route.loader`/`match.route.action` directly - but we have since decided against that with the `match.handler` API. | ||
|
|
||
| ⚠️ `type` was eliminated in favor of `match.handler`. | ||
|
|
||
| `dataStrategy` is control _when_ handlers are called, not _how_. RR is in charge of calling them with the right parameters. | ||
|
|
||
| ### Outputs | ||
|
|
||
| Originally, we planned on making the `DataResult` API public, which is a union of the different types of results (`SuccessResult`, `ErrorResult`, `RedirectResult`, `DeferResult`). However, as we kept evolving and did some internal refactoring to separate calling loaders from decoding results - we realized that all we really need is a simpler `HandlerResult`: | ||
|
|
||
| ```ts | ||
| interface HandlerResult { | ||
| type: ResultType.success | ResultType.error; | ||
| result: any; | ||
| } | ||
| ``` | ||
|
|
||
| If the user returns us one of those per-match, we can internally convert it to a `DataResult`. | ||
|
|
||
| - If `result` is a `Response` then we can handle unwrapping the data and processing any redirects (may produce a `SuccessResult`, `ErrorResult`, or `RedirectResult`) | ||
| - If `result` is a `DeferredData` instance, convert to `DeferResult` | ||
| - If result is anything else we don't touch the data, it's either a `SuccessResult` or `ErrorResult` based on `type` | ||
| - This is important because it's lets the end user opt into a different decoding strategy of their choice. If they return us a Response, we decode it. If not, we don't touch it. | ||
|
|
||
| ### Decoding Responses | ||
|
|
||
| Initially, we intended for `dataStrategy` to handle (3), and considered an optional `decodeResponse` API for (4) - but we decided that the decoding of responses was a small enough undertaking using standard Fetch APIs (i.e., `res.json`) that it didn't warrant a custom property - and they could just call those APIs directly. The `defaultStrategy` parameter would handle performing 3 the normal way that RR would. | ||
|
|
||
| ⚠️ `decodeResponse` is made obsolete by `HandlerResult` | ||
|
|
||
| ### Handling `route.lazy` | ||
|
|
||
| There's a nuanced step we missed in our sequential steps above. If a route was | ||
| using `route.lazy`, we may need to load the rout before we can execute the `loader`. There's two options here: | ||
|
|
||
| 1. We pre-execute all `route.lazy` methods before calling `dataStrategy` | ||
| 2. We let `dataStrategy` execute them accordingly | ||
|
|
||
| (1) has a pretty glaring perf issue in that it blocks _any_ loaders from running until _all_ `route.lazy`'s have resolved. So if route A is super small but has a slow loader, and route B is large but has a fast loader: | ||
|
|
||
| ``` | ||
| |-- route a lazy --> |-- route a loader --------------->| | ||
| |-- route b lazy ------------------------>|-- route b loader --> | | ||
| ``` | ||
|
|
||
| This is no bueno. Instead, we want option (2) where the users can run these sequentially per-route - and "loading the route" is just part of the "loading the data" step | ||
|
|
||
| ``` | ||
| |-- route a lazy -->|-- route a loader ---------------> | | ||
| |-- route b lazy ------------------------>|-- route b loader -->| | ||
| ``` | ||
|
|
||
| Therefore, we're introducing the concept of a `DataStrategyMatch` which is just like a `RouteMatch` but the `match.route` field is a `Promise<Route>`. We'll kick off the executions of route.lazy and then you can wait for them to complete prior to calling the loader: | ||
|
|
||
| ```js | ||
| function dataStrategy({ matches, defaultStrategy }) { | ||
| return Promise.all( | ||
| matches.map((m) => match.route.then((route) => route.loader(/* ... */))) | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
| There are also statically defined properties that live outside of lazy, so those are extended right onto `match.route`. This allows you to define loaders statically and run them in parallel with `route.lazy`: | ||
|
|
||
| ```js | ||
| function dataStrategy({ matches, defaultStrategy }) { | ||
| // matches[0].route => Promise | ||
| // matches[0].route.id => string | ||
| // matches[0].route.index => boolean | ||
| // matches[0].route.path => string | ||
| } | ||
| ``` | ||
|
|
||
| ⚠️ We are actively seeing if we can eliminate this via `match.handler` | ||
|
|
||
| ### Handling `shouldRevalidate` behavior | ||
|
|
||
| We considered how to handle `shouldRevalidate` behavior. There's sort of 2 basic approaches: | ||
|
|
||
| 1. We pre-filter and only hand the user `matchesToLoad` | ||
| 2. We hand the user all matches and let them filter | ||
| - This would probably also require a new `defaultShouldRevalidate(match) => boolean` parameter passed to `dataStrategy` | ||
|
|
||
| I _think_ (1) is preferred to keep the API at a minimum and avoid leaking into _other_ ways to opt-out of revalidation. We already have an API for that so let's lean into it. | ||
|
|
||
| Additionally, another big con of (2) is that if we want to let them make revalidation decisions inside `dataStrategy` - we need to expose all of the informaiton required for that (`currentUrl`, `currentParams`, `nextUrl`, `nextParams`, `submission` info, `actionResult`, etc.) - the API becomes a mess. | ||
|
|
||
| Therefore we are aiming to stick with one and let `shouldRevalidate` be the only way to opt-out of revalidation. | ||
|
|
||
| ### Handling actions and fetchers | ||
|
|
||
| Thus far, we've been mostly concerned with how to handle navigational loaders where they are multiple matched routes and loaders to run. But what about actions and fetchers where we only run a handler for a single leaf match? The quick answer to this is to just send a single-length array with the match in question: | ||
|
|
||
| ```js | ||
| // loaders | ||
| let matchesToLoad = getMatchesToLoad(request, matches); | ||
| let results = await dataStrategy({ | ||
| request, | ||
| params, | ||
| matches: matchesToLoad, | ||
| type: "loader", | ||
| defaultStrategy, | ||
| }); | ||
|
|
||
| // action | ||
| let actionMatch = getTargetMatch(request, matches); | ||
| let actionResults = await dataStrategy({ | ||
| request, | ||
| params, | ||
| matches: [actionMatch], | ||
| type: "action", | ||
| defaultStrategy, | ||
| }); | ||
| let actionResult = actionResults[0]; | ||
|
|
||
| // fetcher loader/action | ||
| let fetcherMatch = getTargetMatch(request, matches); | ||
| let fetcherResults = await dataStrategy({ | ||
| request, | ||
| params, | ||
| matches: [fetcherMatch], | ||
| type: "loader", // or "action" | ||
| defaultStrategy, | ||
| }); | ||
| let fetcherResult = fetcherResults[0]; | ||
| ``` | ||
|
|
||
| This way, the user's implementation can just always operate on the `matches` array and it'll work for all use cases. | ||
|
|
||
| ```js | ||
| // Sample strategy to run sequentially | ||
| function dataStrategy({ request, params, matches, type }) { | ||
brophdawg11 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| let results = []; | ||
| for (let match of matches) { | ||
| let result = await match.route[type]({ request, params }); | ||
| result.push(result); | ||
| } | ||
| return results; | ||
| } | ||
| ``` | ||
|
|
||
| ### What about middlewares? | ||
|
|
||
| As we thought more and more about this API, it became clear that the concept of "process data for a route" (step 3 above) was not necessarily limited to the `loader`/`action` and that there are data-related APIs on the horizon such as `middleware` and `context` that would also fall under the `dataStrategy` umbrella! In fact, a well-implemented `dataStrategy` could alleviate the need for first-class APIs - even if only initially. Early adopters could use `dataStrategy` to implement their own middlewares and we could see which patterns rise to the top and adopt them as first class `route.middleware` or whatever. | ||
|
|
||
| So how would middleware work? The general idea is that middleware runs sequentially top-down prior to the loaders running. And if you bring `context` into the equation - they also run top down and middlewares/loaders/actions receive the context from their level and above in the tree - but they do not "see" any context from below them in the tree. | ||
|
|
||
| A user-land implementation turns out not to be too bad assuming routes define `middleware`/`context` on `handle`: | ||
|
|
||
| ```js | ||
| // Assume routes look like this: | ||
| let route = { | ||
| id: "parent", | ||
| path: "/parent", | ||
| loader: () => {}, | ||
| handle: { | ||
| // context can provide multiple keyed contexts | ||
| context: { | ||
| parent: () => ({ id: "parent" }), | ||
| }, | ||
| // middleware receives context as an argument | ||
| middleware(context) { | ||
| context.parent.whatever = "PARENT MIDDLEWARE"; | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| async function dataStrategy({ request, params, matches, type }) { | ||
| // Run context/middleware sequentially | ||
| let contexts = {}; | ||
| for (let match of matches) { | ||
| if (m.route.handle?.context) { | ||
| for (let [id, ctx] of Object.entries(m.route.handle.context)) { | ||
| contexts[key] = ctx(); | ||
| } | ||
| } | ||
| if (m.route.handle?.middleware) { | ||
| m.route.handle.middleware(context); | ||
| } | ||
| } | ||
|
|
||
| // Run loaders in parallel (or run the solo action) | ||
| return Promise.all( | ||
| matches.map(async (m, i) => { | ||
| // Only expose contexts from this level and above | ||
| let context = matches.slice(0, i + 1).reduce((acc, m) => { | ||
| Object.keys(m.route.handle?.context).forEach((k) => { | ||
| acc[k] = contexts[k]; | ||
| }); | ||
| return acc; | ||
| }, {}); | ||
| try { | ||
| return { | ||
| type: ResultType.data, | ||
| data: await m.route[type]?.({ request, params, context });, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| type: ResultType.error, | ||
| error, | ||
| }; | ||
| } | ||
| }) | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
| ❌ Nope - this doesn't actually work! | ||
|
|
||
| Remember above where we decided to _pre-filter_ the matches based on `shouldRevalidate`? That breaks any concept of middleware since even if we don't intend to load a route, we need to run middleware on all parents before the loader. So we _must_ expose at least the `matches` at or above that level in the tree - and more likely _all_ matches to `dataStrategy` if it's to be able to implement middleware. | ||
|
|
||
| And then, once we expose _multiple_ matches - we need to tell the user if they're supposed to actually run the handlers on those matches or only on a leaf/target match. | ||
|
|
||
| I think there's a few options here: | ||
|
|
||
| **Option 1 - `routeMatches` and `handlerMatches`** | ||
|
|
||
| We could add a second array of the "full" set of matches for the route and then middleware would operate on that set, and handlers would operate on the filtered set (renamed to `handlerMatches`) here. This still preserves the pre-filtering and keeps `shouldRevalidate` logic out of `dataStrategy`. | ||
|
|
||
| ```js | ||
| async function dataStrategy({ request, params, routeMatches, handlerMatches, type }) { | ||
| // Run context/middleware sequentially | ||
| let contexts = {}; | ||
| for (let match of routeMatches) { ... } | ||
|
|
||
| // Run loaders in parallel | ||
| return Promise.all( | ||
| handlerMatches.map(async (m, i) => { ... }) | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
| **Option 2 - new field on `DataStrategyMatch`** | ||
|
|
||
| Since we're already introducing a concept of a `DataStrategyMatch` to handle `route.lazy`, we could lean into that and expose something on those matches that indicate if they need to have their handler run or not? | ||
|
|
||
| ```js | ||
| // Inside React Router, assume navigate from /a/ -> /b and we don't need to | ||
| // re-run the root loader | ||
| let dataStrategyMatches = [{ | ||
| route: { id: 'root', loader() {}, ... } | ||
| runHandler: false // determined via shouldRevalidate | ||
| }, { | ||
| route: { id: 'b', loader() {}, ... } | ||
| runHandler: true // determined via shouldRevalidate | ||
| }] | ||
| ``` | ||
|
|
||
| Then, the user could use this to differentiate between middlewares and handlers: | ||
|
|
||
| ```js | ||
| async function dataStrategy({ request, params, matches, type }) { | ||
| // Run context/middleware sequentially | ||
| let contexts = {}; | ||
| for (let match of matches) { ... } | ||
|
|
||
| // Run loaders in parallel | ||
| let matchesToLoad = matches.filter(m => m.runHandler); | ||
| return Promise.all( | ||
| matchesToLoad.map(async (m, i) => { ... }) | ||
| ); | ||
| } | ||
| ``` | ||
|
|
||
| **Option 3 - new function on `DataStrategyMatch`** | ||
|
|
||
| Extending on the idea above - it all started to feel super leaky and full of implementation-details. Why are users manually filtering? Or manually passing parameters to loaders/actions? Using a `type` field to know which to call? Waiting on a `match.route` Promise before calling the loader? | ||
|
|
||
| That's wayyyy to many rough edges for us to document and users to get wrong (rightfully so!). | ||
|
|
||
| Why can't we just do it all? LE'ts wrap _all_ of that up into a single `match.handler()` function that: | ||
|
|
||
| - Waits for `route.lazy` to resolve (if needed) | ||
| - No-ops if the route isn't supposed to revalidate | ||
| - Open question here if we return the _current_ data from these no-ops or return `undefined`? | ||
| - Knows whether to call the `loader` or the `action` | ||
| - Allows users to pass _additional_ params to loaders/actions for middleware/context use cases. | ||
|
|
||
| ```js | ||
| function dataStrategy({ matches }) { | ||
| // No more type, defaultStrategy, or match.route promise APIs! | ||
|
|
||
| // Can implement middleware as above since you now get all matches | ||
| let context = runMiddlewares(matches); | ||
|
|
||
| // Call all loaders in parallel (no params to pass) but you _can_ pass you | ||
| // own argument to `handler` and it will come in as `loader({ request }, handlerArg)` | ||
| // So you can send middleware context through to loaders/actions | ||
| return Promise.all(matches.map(m => m.handler(context)); | ||
|
|
||
| // Note we don't do any filtering above - if a match doesn't need to load, | ||
| // `match.handler` is no-op. Just like `serverLoader` is a no-op in `clientLoader` | ||
| // when it doesn't need to run | ||
| } | ||
| ``` | ||
|
|
||
| ## Consequences | ||
|
|
||
| [single-fetch-issue]: https://github.com/remix-run/remix/issues/7641 | ||
| [single-fetch-rfc]: https://github.com/remix-run/remix/discussions/7640 | ||
| [responsibilities-comment]: https://github.com/remix-run/remix/issues/7641#issuecomment-1836635069 | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.