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

Make pre-routed request.route property null #4433

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kanongil
Copy link
Contributor

This fixes the value of request.route during onRequest processing to closer match the docs. As it is, it just returns the notFound route. I updated the docs to clarify that the value will be null.

The PR ended up being quite extensive, since the internal logic uses the route property when it has not been routed. I fixed this by using the internal _route instead.

This PR also contains a fix for a minor issue with response.escape(true), which would be ignored.

@kanongil kanongil added the bug Bug or defect label Mar 12, 2023
@kanongil
Copy link
Contributor Author

Note that this is a bug fix to clarify the public API and make the code match this. It might however cause problems for code that uses onRequest extensions that inadvertently relies on the request.route value being set.

@devinivy
Copy link
Member

For that reason I wonder if we should save this for the next major. I figure, if folks do currently have this use-case then it will cause a breakage for them, if if they don't have this use-case then they are not experiencing an issue. I suppose the upside here would be that this helps folks who think they have working code but actually do not, due to the ambiguous documentation.

Hmm! What do others think?

@kanongil kanongil added documentation Non-code related changes breaking changes Change that can breaking existing code labels Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Change that can breaking existing code bug Bug or defect documentation Non-code related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants