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

Missing migration path for deprecated responseJSON and responseText property #2923

Open
BobrImperator opened this issue Jan 13, 2025 · 4 comments
Labels

Comments

@BobrImperator
Copy link
Collaborator

During typescript migration we've noted that responseJSON and responseText properties returned from OAuth2PasswordGrantAuthenticator are deprecated. However I've overlooked the fact that there's no clear migration path for this.

Originally reported here: #2883 (comment)

What are the alternatives for reponseJSON and reponseText? I use these to show the error returned by the backend on login and since the response you return has already been read, it's not an option to use that one.\

Is there any chance you could return a copy of the response (before it was read)? (or copy the response, read that one and send the original)

Draft: #2922

@BoussonKarel
Copy link

BoussonKarel commented Jan 13, 2025

This is not what I meant by cloning the response to access the body:

const response = await fetch(url, options);
const text = await response.text();
const cloned = response.clone() as OAuth2Response;
try {
   const json = JSON.parse(text);
   if (response.ok) {
      return json;
   } else {
      cloned.responseJSON = json;
      throw cloned;
   }
} catch (SyntaxError) {
   cloned.responseText = text;
   throw cloned;
}

I was thinking about something like this:

const response = await fetch(url, options);
// Clone before reading, so user can also read this cloned response, without getting error about body being used
const cloned = response.clone() as OAuth2Response;
// Read original response
const text = await response.text();
try {
   const json = JSON.parse(text);
   if (response.ok) {
      return json;
   } else {
      // We don't need this, the user can just read the cloned response using response.text() or response.json()
      // cloned.responseJSON = json;
      throw cloned;
   }
} catch (SyntaxError) {
   // We don't need this, the user can just read the cloned response using response.text() or response.json()
   // cloned.responseText = text;
   throw cloned;
}

This removes the responseJSON and responseText that normally don't exist on a response + the user can still read the original body using response.text() and response.json() (instead of accessing these properties on the response they read it like any other response).

@BobrImperator
Copy link
Collaborator Author

Unless I'm not catching onto something, the corrected code is pretty much the same right?

I believe that the draft PR does what you're asking for i.e.:

  • Adds ability to call json() and text() on response.
  • Adds responseJSON, responseText so there's no need to commit a breaking change for now.
    We'd like to provide migration paths in between major versions ideally.

For now I'd like to try to resolve it without making a breaking change, but if it'll be too cumbersome then we might drop them with the next release.

@BoussonKarel
Copy link

Your PR is almost correct, you just need to place the clone() before the text() method.
Otherwise, you're cloning an already read response, so the cloned one can also not be read.

@BoussonKarel
Copy link

I also think it's not possible to clone an already read response, see https://developer.mozilla.org/en-US/docs/Web/API/Response/clone#:~:text=clone()-,throws%20a%20TypeError,-if%20the%20response

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

No branches or pull requests

2 participants