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

Keep clientAddress on cloned requests #12613

Merged
merged 6 commits into from
Dec 5, 2024
Merged

Keep clientAddress on cloned requests #12613

merged 6 commits into from
Dec 5, 2024

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Dec 3, 2024

User observed that calling actions resulted in an error about not having clientRequest available.

This is because the user had a middleware that cloned the request, which loses all of the symbols.

Changes

  • The fix is to pass the clientAddress directly into the RenderContext. This deprecates the clientAddressSymbol, but we need to keep it for now because some adapters set the clientAddress that way.
  • Note that similar fixes should be done for other symbol usage on the Request object (locals is one).

Testing

  • Middleware added to the client address fixture which recreates the problem.

Docs

N/A, bug fix.

User observed that calling actions resulted in an error about not having
clientRequest available.

This is because the user had a middleware that cloned the request, which
loses all of the symbols.

The fix is to pass the clientAddress directly into the RenderContext.
This deprecates the `clientAddressSymbol`, but we need to keep it for
now because some adapters set the clientAddress that way.

Note that similar fixes should be done for other symbol usage on the
Request object (locals is one).
Copy link

changeset-bot bot commented Dec 3, 2024

🦋 Changeset detected

Latest commit: b3d32e6

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

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Dec 3, 2024
Copy link

codspeed-hq bot commented Dec 3, 2024

CodSpeed Performance Report

Merging #12613 will not alter performance

Comparing keep-client-address (b3d32e6) with main (b073014)

Summary

✅ 4 untouched benchmarks

Comment on lines 5 to 18
Keep clientAddress on cloned requests

User observed that calling actions resulted in an error about not having
clientRequest available.

This is because the user had a middleware that cloned the request, which
loses all of the symbols.

The fix is to pass the clientAddress directly into the RenderContext.
This deprecates the `clientAddressSymbol`, but we need to keep it for
now because some adapters set the clientAddress that way.

Note that similar fixes should be done for other symbol usage on the
Request object (locals is one).
Copy link
Member

Choose a reason for hiding this comment

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

Can we reword the changeset for the users? This one contains a lot of information that users might not understand or don't need.

We might want to highlight the actual deprecation though, and maybe log a warning if the user attempts to use the old value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will reword the changeset. I don't think we should start spitting out warnings this early in a major though. We definitely at least want to update all of the adapters first to not set this symbol.

@ematipico ematipico merged commit 306c9f9 into main Dec 5, 2024
16 checks passed
@ematipico ematipico deleted the keep-client-address branch December 5, 2024 13:10
@astrobot-houston astrobot-houston mentioned this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants