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

Simplify the setting of pageKey #73

Merged
merged 1 commit into from
Jun 23, 2024
Merged

Simplify the setting of pageKey #73

merged 1 commit into from
Jun 23, 2024

Conversation

jho406
Copy link
Collaborator

@jho406 jho406 commented Jun 22, 2024

This commit makes Superglue a bit smarter. Previously, content-location was used to specify the pageKey for 200s from non-get requests, but this was too cumbersome as the most common use case was setting the URL to be the referrer of the non-get request upon 200.

This uses that as the default logic but still accepts content-location if we want to explicitly override from the server, which is unlikely. Additionally I simplified the logic. We always want to use the response's URL unless in the case mentioned above. Existing tests pass.

This commit makes Superglue a bit smarter. Previously, content-location was
used to specify the pageKey for 200s from non-get requests, but this was too
cumbersome as the most common use case was setting the URL to be the referrer
of the non-get request upon 200.

This uses that as the default logic but still accepts content-location if
we want to explicitly override from the server, which is unlikely. Additionally
I simplified the logic. We always want to use the response's URL unless in
the case mentioned above. Existing tests pass.
@jho406 jho406 force-pushed the simplify-location branch from 8765215 to 6b4ea55 Compare June 23, 2024 00:41
@jho406
Copy link
Collaborator Author

jho406 commented Jun 23, 2024

@sallyhall I'm going to merge this in. We can do a post review 😄

@jho406 jho406 merged commit c89b123 into main Jun 23, 2024
15 checks passed
Copy link

@sallyhall sallyhall left a comment

Choose a reason for hiding this comment

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

🎉 Love this.

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.

2 participants