Skip to content

Patch patch cause the spec is weird #10216

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.

Sign up for GitHub

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 2 commits into from
Mar 17, 2023
Merged

Patch patch cause the spec is weird #10216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ffd1c5
Patch patch cause the spec is weird
brophdawg11 Mar 16, 2023
ec14142
Update to latest @remix-run/web-fetch package
brophdawg11 Mar 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/update-web-fetch.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@remix-run/router": patch
---

Update to latest `@remix-run/[email protected]`
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"@octokit/plugin-paginate-rest": "^2.17.0",
"@octokit/rest": "^18.12.0",
"@remix-run/changelog-github": "^0.0.5",
"@remix-run/web-fetch": "4.1.3",
"@remix-run/web-fetch": "4.3.3",
"@rollup/plugin-babel": "^5.3.1",
"@rollup/plugin-replace": "^4.0.0",
"@rollup/plugin-typescript": "^8.3.2",
Expand Down
82 changes: 54 additions & 28 deletions packages/router/__tests__/router-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ function setup({
// Otherwise we should only need a loader for the leaf match
let activeLoaderMatches = [match];
// @ts-expect-error
if (opts?.formMethod === "post") {
if (opts?.formMethod != null && opts.formMethod.toLowerCase() !== "get") {
if (currentRouter.state.navigation?.location) {
let matches = matchRoutes(
inFlightRoutes || currentRouter.routes,
Expand Down Expand Up @@ -689,7 +689,7 @@ function setup({
invariant(currentRouter, "No currentRouter available");

// @ts-expect-error
if (opts?.formMethod === "post") {
if (opts?.formMethod != null && opts.formMethod.toLowerCase() !== "get") {
activeActionType = "navigation";
activeActionNavigationId = navigationId;
// Assume happy path and mark this navigations loaders as active. Even if
Expand Down Expand Up @@ -779,7 +779,7 @@ function setup({
invariant(currentRouter, "No currentRouter available");

// @ts-expect-error
if (opts?.formMethod === "post") {
if (opts?.formMethod != null && opts.formMethod.toLowerCase() !== "get") {
activeActionType = "fetch";
activeActionFetchId = navigationId;
} else {
Expand Down Expand Up @@ -867,10 +867,7 @@ function initializeTmTest(init?: {
}

function createRequest(path: string, opts?: RequestInit) {
return new Request(`http://localhost${path}`, {
signal: new AbortController().signal,
...opts,
});
return new Request(`http://localhost${path}`, opts);
}

function createSubmitRequest(path: string, opts?: RequestInit) {
Expand Down Expand Up @@ -5899,6 +5896,47 @@ describe("a router", () => {
expect((await request.formData()).get("query")).toBe("params");
});

// https://fetch.spec.whatwg.org/#concept-method
it("properly handles method=PATCH weirdness", async () => {
Copy link
Contributor Author

@brophdawg11 brophdawg11 Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This test doesn't fail (without the code fix) at the moment - I think our fetch polyfill is probably uppercasing all methods, so we should update that to force this test to be able to fail

Copy link
Contributor Author

@brophdawg11 brophdawg11 Mar 17, 2023

Choose a reason for hiding this comment

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

remix-run/web-std-io#30

let t = setup({
routes: TASK_ROUTES,
initialEntries: ["/"],
hydrationData: {
loaderData: {
root: "ROOT_DATA",
},
},
});

let nav = await t.navigate("/tasks", {
formMethod: "patch",
formData: createFormData({ query: "params" }),
});
expect(nav.actions.tasks.stub).toHaveBeenCalledWith({
params: {},
request: expect.any(Request),
});

// Assert request internals, cannot do a deep comparison above since some
// internals aren't the same on separate creations
let request = nav.actions.tasks.stub.mock.calls[0][0].request;
expect(request.method).toBe("PATCH");
expect(request.url).toBe("http://localhost/tasks");
expect(request.headers.get("Content-Type")).toBe(
"application/x-www-form-urlencoded;charset=UTF-8"
);
expect((await request.formData()).get("query")).toBe("params");

await nav.actions.tasks.resolve("TASKS ACTION");
let rootLoaderRequest = nav.loaders.root.stub.mock.calls[0][0].request;
expect(rootLoaderRequest.method).toBe("GET");
expect(rootLoaderRequest.url).toBe("http://localhost/tasks");

let tasksLoaderRequest = nav.loaders.tasks.stub.mock.calls[0][0].request;
expect(tasksLoaderRequest.method).toBe("GET");
expect(tasksLoaderRequest.url).toBe("http://localhost/tasks");
});

it("handles multipart/form-data submissions", async () => {
let t = setup({
routes: [
Expand Down Expand Up @@ -13437,17 +13475,12 @@ describe("a router", () => {
expect(e).toMatchInlineSnapshot(`[Error: query() call aborted]`);
});

it("should require a signal on the request", async () => {
it("should assign signals to requests by default (per the", async () => {
let { query } = createStaticHandler(SSR_ROUTES);
let request = createRequest("/", { signal: undefined });
let e;
try {
await query(request);
} catch (_e) {
e = _e;
}
expect(e).toMatchInlineSnapshot(
`[Error: query()/queryRoute() requests must contain an AbortController signal]`
let context = await query(request);
expect((context as StaticHandlerContext).loaderData.index).toBe(
"INDEX LOADER"
);
});

Expand Down Expand Up @@ -14673,18 +14706,11 @@ describe("a router", () => {
expect(e).toMatchInlineSnapshot(`[Error: queryRoute() call aborted]`);
});

it("should require a signal on the request", async () => {
it("should assign signals to requests by default (per the spec)", async () => {
let { queryRoute } = createStaticHandler(SSR_ROUTES);
let request = createRequest("/", { signal: undefined });
let e;
try {
await queryRoute(request, { routeId: "index" });
} catch (_e) {
e = _e;
}
expect(e).toMatchInlineSnapshot(
`[Error: query()/queryRoute() requests must contain an AbortController signal]`
Copy link
Contributor Author

@brophdawg11 brophdawg11 Mar 17, 2023

Choose a reason for hiding this comment

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

We've since fixed our Request implementation to match the spec so it always has a signal. It's no longer possible to create one without a signal so altered these tests to just confirm that

);
let data = await queryRoute(request, { routeId: "index" });
expect(data).toBe("INDEX LOADER");
});

it("should support a requestContext passed to loaders and actions", async () => {
Expand Down Expand Up @@ -14890,15 +14916,15 @@ describe("a router", () => {

it("should handle unsupported methods with a 405 Response", async () => {
try {
await queryRoute(createRequest("/", { method: "TRACE" }), {
await queryRoute(createRequest("/", { method: "CHICKEN" }), {
routeId: "root",
});
expect(false).toBe(true);
} catch (data) {
expect(isRouteErrorResponse(data)).toBe(true);
expect(data.status).toBe(405);
expect(data.error).toEqual(
new Error('Invalid request method "TRACE"')
new Error('Invalid request method "CHICKEN"')
);
expect(data.internal).toBe(true);
}
Expand Down
5 changes: 4 additions & 1 deletion packages/router/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3492,7 +3492,10 @@ function createClientSideRequest(

if (submission && isMutationMethod(submission.formMethod)) {
let { formMethod, formEncType, formData } = submission;
init.method = formMethod;
// Didn't think we needed this but it turns out unlike other methods, patch
// won't be properly normalized to uppercase and results in a 405 error.
// See: https://fetch.spec.whatwg.org/#concept-method
init.method = formMethod.toUpperCase();
init.body =
formEncType === "application/x-www-form-urlencoded"
? convertFormDataToSearchParams(formData)
Expand Down
19 changes: 10 additions & 9 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1997,22 +1997,23 @@
"@remix-run/web-stream" "^1.0.0"
web-encoding "1.1.5"

"@remix-run/web-fetch@4.1.3":
version "4.1.3"
resolved "https://registry.yarnpkg.com/@remix-run/web-fetch/-/web-fetch-4.1.3.tgz#8ad3077c1b5bd9fe2a8813d0ad3c84970a495c04"
integrity sha512-D3KXAEkzhR248mu7wCHReQrMrIo3Y9pDDa7TrlISnsOEvqkfWkJJF+PQWmOIKpOSHAhDg7TCb2tzvW8lc/MfHw==
"@remix-run/web-fetch@4.3.3":
version "4.3.3"
resolved "https://registry.yarnpkg.com/@remix-run/web-fetch/-/web-fetch-4.3.3.tgz#708371a43f20e645090150dfadb983e950bff12d"
integrity sha512-DK9vA2tgsadcFPpxW4fvN198tiWpyPhwR0EYOuM4QjpDCz0G619c9RDMdyMy6a7Qb/jwiyx9SOPHWc65QAl+1g==
dependencies:
"@remix-run/web-blob" "^3.0.4"
"@remix-run/web-form-data" "^3.0.2"
"@remix-run/web-form-data" "^3.0.3"
"@remix-run/web-stream" "^1.0.3"
"@web3-storage/multipart-parser" "^1.0.0"
abort-controller "^3.0.0"
data-uri-to-buffer "^3.0.1"
mrmime "^1.0.0"

"@remix-run/web-form-data@^3.0.2":
version "3.0.3"
resolved "https://registry.yarnpkg.com/@remix-run/web-form-data/-/web-form-data-3.0.3.tgz#f89a7f971aaf1084d2da87affbb7f4e01c32b8ce"
integrity sha512-wL4veBtVPazSpXfPMzrbmeV3IxuxCfcQYPerQ8BXRO5ahAEVw23tv7xS+yoX0XDO5j+vpRaSbhHJK1H5gF7eYQ==
"@remix-run/web-form-data@^3.0.3":
version "3.0.4"
resolved "https://registry.yarnpkg.com/@remix-run/web-form-data/-/web-form-data-3.0.4.tgz#18c5795edaffbc88c320a311766dc04644125bab"
integrity sha512-UMF1jg9Vu9CLOf8iHBdY74Mm3PUvMW8G/XZRJE56SxKaOFWGSWlfxfG+/a3boAgHFLTkP7K4H1PxlRugy1iQtw==
dependencies:
web-encoding "1.1.5"

Expand Down