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

[Bug]: Support non-standard HTTP methods in Resource Routes #11959

Closed
alexanderson1993 opened this issue Sep 5, 2024 · 3 comments
Closed

[Bug]: Support non-standard HTTP methods in Resource Routes #11959

alexanderson1993 opened this issue Sep 5, 2024 · 3 comments
Labels

Comments

@alexanderson1993
Copy link

What version of React Router are you using?

React Router 6.24 / Remix 2.11

Steps to Reproduce

  • Create a resource route like so
export function loader({ request }: LoaderFunctionArgs) {
  console.log('Loader Method', request.method);

  return 'done';
}

export function action({ request }: ActionFunctionArgs) {
  console.log('Action Method', request.method);

  return 'done';
}

Make an HTTP request using a non-standard HTTP verb, like PROPFIND and SEARCH, both standardized for WebDAV use: https://www.rfc-editor.org/rfc/rfc5323

curl http://localhost:5173/webdav -X SEARCH

Expected Behavior

The resource route should appropriately handle the request. Whether it handles the request through the loader or action is an open question.

Addressing this involves changing the implementation of the queryRoute function in router.ts, either to

  • loosen up the methods check altogether so any method is allowed (though with the current implementation, all non-standard methods would be handled by loader), or
  • add specific non-standard HTTP methods to the validMutationMethodsArr and validRequestMethodsArr, (which solves the problem of whether certain methods are handled by loader or action), or
  • adding extra exports to resource routes which tell Remix which HTTP methods that resource route will accept, perhaps something like export const loaderMethods = ["GET", "SEARCH"], or
  • make a breaking change for resource routes to export a function resource(){} which handles all HTTP methods instead of loader and action, and allow all methods to be handled by that function.

I lean towards adding specific HTTP methods. Adding loaderMethods as an export adds to the API surface area, though it's probably not a huge deal since the current default covers 99.9% of cases. Breaking resource routes with a function resource() export is a huge breaking change and likely too painful for most users (and I'm sure the team has discussed this option before as well)

Methods that are not currently implemented, but are included in the HTTP spec include:

WebDAV methods include:

  • COPY
  • LOCK
  • UNLOCK
  • MKCOL
  • MOVE
  • PROPFIND
  • PROPPATCH

Another alternative is to close this issue and expect developers to implement these non-standard HTTP methods in the underlying HTTP server instead of using Remix Resource Routes.

Actual Behavior

Remix throws an error: Error: Invalid request method "SEARCH"

@brophdawg11
Copy link
Contributor

Whether it handles the request through the loader or action is an open question

It looks like these new ones have fairly well defined semantics - https://www.ibm.com/docs/en/i/7.5?topic=concepts-webdav seems to indicate PROPFIND might be the only non-mutating one.

However, if we implement this, I want to avoid having to continually update the list of methods in the future as new ones might come around, so I would probably vote for a looser implementation of:

  • Resource routes accept any method
  • We send any unknown methods (i.e., not included in this list) to the action and make no assumptions about whether or not it mutates anything

I'm still not convinced we should implement this though :). Resource routes are API routes that play nicely within the Remix data patterns. They can be hit from fetchers, use client loaders, leverage single fetch, etc. A Remix request (from a fetcher or useSubmit) will never send one of those non-standard methods. So you could argue that non-standard method behavior belongs at the server level outside of Remix since Remix will never be able to leverage it.

@alexanderson1993
Copy link
Author

So you could argue that non-standard method behavior belongs at the server level outside of Remix since Remix will never be able to leverage it.

I basically convinced myself of this as I wrote the issue 😅

I'll leave it to you and the team whether you want to implement this or not, but now you've got something concrete you can point people towards when they ask why resource routes don't support non-standard HTTP methods.

@brophdawg11
Copy link
Contributor

ok cool - I'm going to close this out for now. AFAIK this was the first time that limitation came up so it's not a high-demand thing - if it ever becomes a higher requested issue we can certainly revisit. Thanks for taking the time to dig in!

@brophdawg11 brophdawg11 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants