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

Add Router option 'automatic405' that emits a 405 Method Not Allowed error upon valid path with no matching method #106

Closed
wants to merge 16 commits into from

Conversation

zachblume
Copy link

I have seen past PRs and several past issues related to this. 405's are useful for debugging/testing so I found this worthwhile.

Here's what I did:

  • added an option automatic405 to Router. If not enabled by the developer, ensures that this PR makes no breaking change
  • added a block to test for 405s just before done() in a 404 type situation where there is 'no match' (the changed code is not spread out throughout the file),
  • added a test for a 405 situation when automatic405 is set to true
  • made sure that 405 also emits the require ALLOW header for valid methods
  • added a test for that as well
  • ran all (698) tests with automatic405 set to true to make sure they all pass, and made some modifications due to 'undefined' variable issues and made sure that .all and .use catch all cases don't break

I know this issue has been kicking around for a long time, and that several people have tried and left off PRs so hoping this is a useful and well-tested contribution for people who choose to have broad 405 coverage, as opposed to a breaking change!

@hypesystem
Copy link

I would prefer a config field name that uses words (e.g. "MethodNotFound" instead of "405"). That seems more consistent with existing options.

But I'm not an authority on the design here so don't make changes based just on my opinion 😄

@zachblume
Copy link
Author

Bumping this to consider merging! Today heard that someone at work spent a day fruitlessly trying to debug a route 404, when a 405 would've hat tipped them to the method switch typo they made.

@wesleytodd
Copy link
Member

I am not entirely sure, but I think this is not a feature for the router but instead would be a feature for frameworks using the router (ex. express). I don't know if you had seen this comment which suggests a way to do it today, but even if we added something I think it would not be on the repo. If you want to continue discussion on this topic I suggest moving it to express and then looking for the past discussions to link to. I will happily close all the others and defer to your proposal for a solution there.

@wesleytodd wesleytodd closed this Mar 16, 2024
@zachblume
Copy link
Author

I will take a look at implementing in Express and write back. Thanks!

@wesleytodd
Copy link
Member

We are working toward the v5 major so I would guess this will not land or be looked at until after. Just want to level set expectations. FWIW it might be better to propose a plan in one of the existing issues around 405 behavior we can make sure it makes sense so you don't waste any of your time. But yeah this sounds great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants