Skip to content

Memoize text, html, json on response? #19

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

Closed
excid3 opened this issue Jun 9, 2021 · 3 comments · Fixed by #21
Closed

Memoize text, html, json on response? #19

excid3 opened this issue Jun 9, 2021 · 3 comments · Fixed by #21

Comments

@excid3
Copy link
Contributor

excid3 commented Jun 9, 2021

I was playing with this today and noticed that a fetch response doesn't allow reading the body twice. To reproduce:

const request = new FetchRequest("post", url)
const response = await request.perform()
await response.text
await response.text
// => Failed to execute 'text' on 'Response': body stream already read

And the situation that caught me was:

const request = new FetchRequest("post", url, { responseKind: "turbo-stream" })
const response = await request.perform()
await response.text
// => Failed to execute 'text' on 'Response': body stream already read

A solution is that we could memoize the text, html, json getters so that you could call this multiple times.

  get text () {
    return this.bodyText || (this.bodyText = this.response.text())
  }

I noticed this because I tried to read the text on a Turbo Stream response. Since renderTurboStream already accessed the text body, it seems unexpected.

Is this something worth fixing?

@KonnorRogers
Copy link
Contributor

+1 not a bad idea. I just implemented this on mrujs without issues. Definitely weird bodies can only be consumed once. Im assuming its some weird limitation on ResponseStreams.

https://github.com/ParamagicDev/mrujs/pull/35/files

Anyways, figured id report it worked for me 🤷

@marcelolx
Copy link
Collaborator

@excid3 @ParamagicDev Yes, it's a good idea, I faced the same problem before extracting the package but didn't addressed the problem at the time.

@excid3
Copy link
Contributor Author

excid3 commented Jun 9, 2021

Great, I'll get a PR started. 👍

excid3 added a commit to excid3/request.js that referenced this issue Jun 10, 2021
marcelolx added a commit that referenced this issue Jun 11, 2021
This saves the `text` and `json` values after reading. Fetch responses don't allow you to read these twice, so it's best if we memoize the results to prevent users from running into this.

```javascript
const request = new FetchRequest("post", url)
const response = await request.perform()
await response.text
await response.text
// => Failed to execute 'text' on 'Response': body stream already read
```

Most importantly, we need memoization so you can still read the response text after the Turbo Stream messages are read and rendered.
```
const request = new FetchRequest("post", url, { responseKind: "turbo-stream" })
const response = await request.perform()
await response.text
// => Failed to execute 'text' on 'Response': body stream already read
```

Fixes #19
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 a pull request may close this issue.

3 participants