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

Return result verbatim if raw=true is provided in the query string #1553

Closed
wants to merge 4 commits into from

Conversation

Tommypop2
Copy link
Contributor

@Tommypop2 Tommypop2 commented Jun 17, 2024

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • infrastructure changes
  • Other... Please describe:

What is the current behavior?

Described in #1550

What is the new behavior?

Can now specify that the response is returned verbatim by adding ?raw=true to the query string

Closes #1550

Copy link

changeset-bot bot commented Jun 17, 2024

⚠️ No Changeset found

Latest commit: 4939754

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Tommypop2 Tommypop2 changed the title Return result verbatim if image/* is included in the Accept header. Return result verbatim if raw=true is provided in the query string Jun 17, 2024
Copy link
Contributor

@OrJDev OrJDev left a comment

Choose a reason for hiding this comment

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

LGTM

@ryansolid
Copy link
Member

So if i get this right.. this is for something like an image tag.. so it falls into the no js case but our setting of flash headers messes things.. or I guess more so us messing with the body encoding with seroval would be the issue. I don't like having to rely on query param.. but I also don't see a particularly great way to handle this in this case.

Our options are to use the query param as you have if the caller makes that decision. Or to do something on the response, if the response makes that decision. It sort of feels like the response knows what it wants sent back. We could use the content-type headers of the response object maybe to make this decision. I guess that is a bit weird.

I'm just unclear all the scenarios here and before adding a convention I'd like to consider them.

@Brendonovich
Copy link
Contributor

IMO we should use the Accept request header here instead, just to account for the case where seroval is wanted.
If Accept has text/javascript then seroval is used, otherwise it's returned verbatim (unless it's a redirect ofc)

@OrJDev
Copy link
Contributor

OrJDev commented Jun 18, 2024

So if i get this right.. this is for something like an image tag.. so it falls into the no js case but our setting of flash headers messes things.. or I guess more so us messing with the body encoding with seroval would be the issue. I don't like having to rely on query param.. but I also don't see a particularly great way to handle this in this case.

Our options are to use the query param as you have if the caller makes that decision. Or to do something on the response, if the response makes that decision. It sort of feels like the response knows what it wants sent back. We could use the content-type headers of the response object maybe to make this decision. I guess that is a bit weird.

I'm just unclear all the scenarios here and before adding a convention I'd like to consider them.

How about a response header

X-Solid-Start-Raw: 1

@ryansolid
Copy link
Member

Yeah the problem with accept headers is things like image URLs can't set them I believe. So yeah response header seems the best option to me. It means being a bit more explicit in these cases but I think that makes sense. The only real question is if we just set one for raw or we leave it open. I guess raw is fine and then rely on the native body parser to handle the rest. We do have to consider both the nojs and js case though so its going to take a bit more wiring. I will take a stab it today I think.

@ryansolid
Copy link
Member

I went with going with "X-Content-Raw". So not going with this PR. Thanks for bring this up to my attention.

@ryansolid ryansolid closed this Jun 18, 2024
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.

[Bug?]: Cannot get response returned by a server function via a GET request
4 participants