Skip to content
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

Update single fetch headers approach to use response stub API #9142

Merged
merged 12 commits into from
Mar 27, 2024

Conversation

brophdawg11
Copy link
Contributor

No description provided.

Copy link

changeset-bot bot commented Mar 26, 2024

🦋 Changeset detected

Latest commit: 6bfd9f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages
Name Type
@remix-run/server-runtime Patch
@remix-run/cloudflare Patch
@remix-run/deno Patch
@remix-run/dev Patch
@remix-run/node Patch
@remix-run/react Patch
@remix-run/testing Patch
@remix-run/cloudflare-pages Patch
@remix-run/cloudflare-workers Patch
@remix-run/architect Patch
@remix-run/express Patch
@remix-run/serve Patch
create-remix Patch
remix Patch
@remix-run/css-bundle Patch
@remix-run/eslint-config Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -16,10 +15,6 @@ export function getDocumentHeaders(
? context.matches.slice(0, boundaryIdx + 1)
: context.matches;

if (loadRouteIds) {
matches = matches.filter((m) => loadRouteIds.includes(m.route.id));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single fetch no longer uses getDocumentHeaders

loadContext,
action,
params,
request,
routeId,
singleFetch,
response,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResponseStub is sent down to all loader/action calls (single fetch only)

@@ -27,6 +27,22 @@ export type DataFunctionArgs = RRActionFunctionArgs<AppLoadContext> &
context: AppLoadContext;
};

export const ResponseStubOperationsSymbol = Symbol("ResponseStubOperations");
export type ResponseStubOperation = [
"set" | "append" | "delete",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only track "write" operations

@@ -406,60 +410,79 @@ async function singleFetchAction(
signal: request.signal,
...(request.body ? { duplex: "half" } : undefined),
});
let result = await staticHandler.queryRoute(handlerRequest, {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

action's use query now as well and pass skipLoaders to just call the action. I think this should let us combine the singleFetchAction/singleFetchLoaders functions eventually too - but didn't look into that yet as part of this work

requestContext: loadContext,
skipLoaderErrorBubbling: true,
skipLoaders: true,
unstable_dataStrategy: getSingleFetchDataStrategy(responseStubs),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We pass a factory-created dataStrategy to query now so we can have a set of responseStub's scoped to this request

Comment on lines +864 to +874
let headersProxy = new Proxy(headers, {
get(target, prop, receiver) {
if (prop === "set" || prop === "append" || prop === "delete") {
return (name: string, value: string) => {
operations.push([prop, name, value]);
Reflect.apply(target[prop], target, [name, value]);
};
}
return Reflect.get(target, prop, receiver);
},
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a Proxy around Headers to track write operations


// Unsure why this is complaining? It's fine in VSCode but fails with tsc...
// @ts-expect-error
for (let v of result.result.headers.getSetCookie()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming this is fine now that we use undici? I know this was one of the areas we deviated from the spec and didn't yet implement this method

@brophdawg11 brophdawg11 merged commit 23e6c0c into dev Mar 27, 2024
9 checks passed
@brophdawg11 brophdawg11 deleted the brophdawg11/single-fetch-headers branch March 27, 2024 14:25
@github-actions github-actions bot added the awaiting release This issue has been fixed and will be released soon label Mar 27, 2024
- `headers.set` on any child handler will overwrite values from parent handlers
- `headers.append` can be used to set the same header from both a parent and child handler
- `headers.delete` can be used to delete a value set by a parent handler, but not a value set from a child handler
- Because single fetch supports naked object returns, and you no longer need to return a `Response` instance to set status/headers, the `json`/`redirect`/`redirectDocument`/`defer` utilities are considered deprecated when using Single Fetch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brophdawg11 , I totally understand that json and defer can be considered deprecated but I'm surprised for redirect and redirectDocument, especially redirectDocument which relies on a Remix custom header, ie an implementation that should be ignored in user land I think. So what will be the recommended way to redirect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, specially because I usually throw redirect responses, to stop execution, will that stop to work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting release This issue has been fixed and will be released soon CLA Signed feat:routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants