Fallback to default injectToStream when react-streaming stream is closed #1718

magne4000 · 2024-06-27T13:38:34Z

PS: I'm not sure why imports are reordered, it's disabled in my IDE (perhaps biome does that?)

brillout · 2024-06-27T13:56:13Z

This would await for the stream to completely end though, see docs about streamEnd at https://github.com/brillout/react-streaming.

Thus, this will probably break progressive rendering. You can test that at /examples/react-streaming. (We should probably add some progressive rendering tests at some point.)

PS: I'm not sure why imports are reordered, it's disabled in my IDE (perhaps biome does that?)

There isn't any automatic formatting, so I'm inclined to think it's something on your side.

magne4000 · 2024-06-27T13:57:42Z

Yup I noticed that's not the fix we need, we would need a way to check if the underlying streamOriginal is closed, but I'm not sure we have that available

brillout · 2024-06-27T13:59:54Z

I wonder why and who is trying to push to the stream even though it's already closed.

…

On Thu, Jun 27, 2024 at 3:58 PM Joël Charles ***@***.***> wrote: Yup I noticed that's not the fix we need, we would need a way to check if the underlying streamOriginal is closed, but I'm not sure we have that available — Reply to this email directly, view it on GitHub <#1718 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHVQRV53OBWK4IBSMVNGDDZJQK63AVCNFSM6AAAAABJ76XYEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJUG42TQMJYGE> . You are receiving this because your review was requested.Message ID: ***@***.***>

magne4000 · 2024-06-27T14:04:47Z

Looking at the log, I would say Vike itself or Vite:

Error: [react-streaming@0.3.29][Wrong Usage] Cannot inject following chunk after stream has ended: `<script id="vike_pageContext" type="application/json">{"$$typeof":"!undefined","abortReason":"!undefined","_urlRewrite":null,"_urlRedirect":"!undefined","abortStatusCode":"!undefined","_abortCall":"!undefined","_pageContextInitIsPassedToClient":"!undefined","_pageId":"/pages/_error","routeParams":{},"data":"!undefined","pageProps":{"is404":false},"is404":false,"_isServerSideError":"!undefined"}</script><script type="module" async>
import RefreshRuntime from "/@react-refresh"
RefreshRuntime.injectIntoGlobalHook(window)
window.$RefreshReg$ = () => {}
window.$RefreshSig$ = () => (type) => type
window.__vite_plugin_react_preamble_installed__ = true
import "/@vite/client";
import "/@fs/tmp/bati-app/node_modules/.pnpm/vike@0.4.177_react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1__vite@5.3.1_@types+node@20.14.9_/node_modules/vike/dist/esm/client/client-routing-runtime/entry.js";
</script>`
    at injectToStream (/tmp/bati-app/node_modules/.pnpm/react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/react-streaming/dist/cjs/server/renderToStream/createBuffer.js:12:33)
    at injectHtmlFragment (file:///tmp/bati-app/node_modules/.pnpm/vike@0.4.177_react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1__vite@5.3.1_@types+node@20.14.9_/node_modules/vike/dist/esm/node/runtime/html/injectAssets/injectHtmlTags.js:48:9)
    at file:///tmp/bati-app/node_modules/.pnpm/vike@0.4.177_react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1__vite@5.3.1_@types+node@20.14.9_/node_modules/vike/dist/esm/node/runtime/html/injectAssets/injectHtmlTags.js:16:26
    at Array.forEach (<anonymous>)
    at injectHtmlTags (file:///tmp/bati-app/node_modules/.pnpm/vike@0.4.177_react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1__vite@5.3.1_@types+node@20.14.9_/node_modules/vike/dist/esm/node/runtime/html/injectAssets/injectHtmlTags.js:10:15)
    at injectToHtmlBegin (file:///tmp/bati-app/node_modules/.pnpm/vike@0.4.177_react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1__vite@5.3.1_@types+node@20.14.9_/node_modules/vike/dist/esm/node/runtime/html/injectAssets.js:41:17)
    at injectAtStreamBegin (file:///tmp/bati-app/node_modules/.pnpm/vike@0.4.177_react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1__vite@5.3.1_@types+node@20.14.9_/node_modules/vike/dist/esm/node/runtime/html/injectAssets.js:25:21)
    at injectStringAtBegin (file:///tmp/bati-app/node_modules/.pnpm/vike@0.4.177_react-streaming@0.3.29_react-dom@18.3.1_react@18.3.1__react@18.3.1__vite@5.3.1_@types+node@20.14.9_/node_modules/vike/dist/esm/node/runtime/html/renderHtml.js:68:24)

brillout · 2024-06-27T14:13:44Z

This chunk is indeed authored by Vike (not Vite).

Both Vike (DEBUG=vike:stream) and react-streaming (brillout/react-streaming@c98b1a5) have debug logs to see what happens when. It's usually an effective way to get to the bottom of these kind of issues. But let me know if you want me to have a look at it.

magne4000 · 2024-06-27T14:36:09Z

But let me know if you want me to have a look at it.

Could be great yes.

I'm not sure if the stream created by react-streaming in createReadableWrapper should be kept open until further notice (but what does it even mean in this case? and how does this then apply to other frameworks supporting streaming?).

Intuitively, I would have injectToStream be a wrapper around any underlying Stream, but it would ignore close (non-error) events from underlying stream (i.e. React stream), and only listen for close events coming from "above" (i.e. Vike or Vite, I'm confused here).

brillout · 2024-06-27T14:58:05Z

How can I reproduce the issue?

magne4000 · 2024-06-27T14:59:02Z

For now I was testing with those instructions vikejs/bati#283 (comment) (and fiddling directly in node_modules)

magne4000 · 2024-06-27T15:18:52Z

I tried with stream: 'node' and:

// vike-handler.ts
import { Writable } from 'node:stream';
...
const { readable, writable } = new TransformStream();

response?.pipe(Writable.fromWeb(writable));

return new Response(readable, {
  status: response?.statusCode,
  headers: response?.headers,
});
...

and no error, so at least it's contained to Web streams mode.

magne4000 · 2024-06-27T15:34:10Z

For now I was testing with those instructions batijs/bati#283 (comment) (and fiddling directly in node_modules)

I just disabled stream mode by default in Bati, so you perhaps need to manually add stream: 'web' back to +config.ts

magne4000 · 2024-06-27T15:46:13Z

Weirdly enough, if I try to copy the reproduction repo in vike/examples I have no issue (but streaming seems disabled) 😕

brillout · 2024-06-27T15:49:17Z

👍 I'll have a look.

magne4000 · 2024-06-27T16:00:12Z

Weirdly enough, if I try to copy the reproduction repo in vike/examples I have no issue (but streaming seems disabled) 😕

🤕 I was testing on this branch with my "fix"...

Anyway, I removed the fix, and pushed the repo in vike/examples/bati-app for testing purpose.

brillout · 2024-06-27T20:25:35Z

I can reproduce.

brillout · 2024-06-27T21:18:15Z

Actually, I cannot reproduce anymore.

Neither on this PR nor on https://github.com/brillout/vike-stream-bug-2024-06. Can you systematically reproduce or is it random?

brillout · 2024-06-27T22:19:38Z

I just pushed this tentative fix on brillout/dev. I'm shooting in the dark as I cannot reproduce; let me know if it fixes the issue (you can test the fix by rebasing this PR onto brillout/dev).

magne4000 · 2024-06-27T22:27:44Z

Actually, I cannot reproduce anymore.

Neither on this PR nor on https://github.com/brillout/vike-stream-bug-2024-06. Can you systematically reproduce or is it random?

Yup still reproduce, even managed get the issue on a second machine

I just pushed this tentative fix on brillout/dev. I'm shooting in the dark as I cannot reproduce; let me know if it fixes the issue (you can test the fix by rebasing this PR onto brillout/dev).

I'll try that

…s closed

magne4000 · 2024-06-27T22:58:15Z

After a quick pnpm dedupe, it at first seems to have fixed the issue (and at least server-side no more error). But it generates an interesting side effect on the client:

The added code after the stream closes makes React confused when trying to rehydrate on the client.

brillout · 2024-06-28T05:32:08Z

That's actually something I was expecting, let me try to reproduce on my machine.

brillout · 2024-06-28T06:57:28Z

I can reproduce. Let me dig & fix.

brillout · 2024-06-28T09:36:48Z

I believe #1722 fixes it.

I ain't sure because I couldn't reproduce the error without the fix. But we can be confident this works if: you can reproduce the error by reverting the fix, then re-apply the fix and then not be able to reproduce the error.

magne4000 · 2024-07-01T08:15:55Z

I do not have issues anymore on #1722 but how can I make sure that streaming is actually working?

brillout · 2024-07-01T08:17:41Z

With working do you mean that it doesn't throw the error anymore or that progressive rendering works?

magne4000 · 2024-07-01T08:19:57Z

With working do you mean that it doesn't throw the error anymore or that progressive rendering works?

The I have no error, but I'm not sure I have progressive rendering, what's the best way to test it?

brillout · 2024-07-01T08:21:44Z

There is a progressive rendering example at /examples/react-streaming/.

brillout · 2024-07-01T08:32:22Z

FYI DEBUG=vike:stream,react-streaming:buffer can be quite helpful.

magne4000 · 2024-07-01T08:37:33Z

@brillout progressive rendering works both with Node Stream and Web Stream with latest vike-react and react-streaming 🚀

brillout · 2024-07-01T08:39:07Z

Even with the universal adapter? 👀

magne4000 · 2024-07-01T08:43:54Z

Yes, that is what I was testing with my latest push 0cc3cbc

brillout · 2024-07-01T08:44:47Z

Nice 🚀

magne4000 · 2024-07-01T08:51:13Z

@brillout I guess that a proper fix requires only to bump vike-react and react-streaming. Do you want me to create another PR for that?

brillout · 2024-07-01T08:56:23Z

Hm, that's surprising as I think #1722 is required to avoid the race condition. There wasn't any fix regarding the race condition error in react-streaming nor vike-react.

I suggest we merge 1722 in the hope it fixes the error, then let's have a look at it again if a user complains.

I'm working on a react-streaming feature right now, then I'll merge 1722.

brillout · 2024-07-03T14:48:05Z

Fixed at #1722 and released in 0.4.178.

magne4000 requested a review from brillout June 27, 2024 13:38

magne4000 mentioned this pull request Jun 27, 2024

Question about react streaming vikejs/bati#283

Closed

magne4000 marked this pull request as draft June 27, 2024 13:45

brillout force-pushed the magne4000/fix-inject-stream branch from ccdf218 to 16d374b Compare June 27, 2024 21:03

fix: avoid injecting to an already closed stream

4973eec

magne4000 and others added 5 commits June 28, 2024 00:48

fix: fallback to default injectToStream when react-streaming stream i…

95f2c9e

…s closed

issue repo

a1a9f8e

issue repo

15c87ee

fmt

8a08d91

pnpm dedupe

c40109e

magne4000 force-pushed the magne4000/fix-inject-stream branch from 16d374b to c40109e Compare June 27, 2024 22:50

magne4000 changed the base branch from main to brillout/dev June 27, 2024 22:50

brillout force-pushed the brillout/dev branch from 4973eec to f0bc61e Compare June 28, 2024 08:24

brillout mentioned this pull request Jun 28, 2024

fix: tolerate injecting to stream after it ended #1722

Merged

update to latest vike-react and react-streaming

0cc3cbc

brillout force-pushed the brillout/dev branch from 56b34a3 to f7f25a2 Compare July 2, 2024 14:58

brillout closed this Jul 3, 2024

magne4000 deleted the magne4000/fix-inject-stream branch July 3, 2024 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fallback to default injectToStream when react-streaming stream is closed #1718

Fallback to default injectToStream when react-streaming stream is closed #1718

magne4000 commented Jun 27, 2024 •

edited

Loading

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024 via email

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024 •

edited

Loading

brillout commented Jun 27, 2024

brillout commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024 •

edited

Loading

brillout commented Jun 28, 2024

brillout commented Jun 28, 2024

brillout commented Jun 28, 2024

magne4000 commented Jul 1, 2024

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024

brillout commented Jul 1, 2024

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024 •

edited

Loading

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024 •

edited

Loading

brillout commented Jul 1, 2024 •

edited

Loading

brillout commented Jul 3, 2024

Fallback to default injectToStream when react-streaming stream is closed #1718

Fallback to default injectToStream when react-streaming stream is closed #1718

Conversation

magne4000 commented Jun 27, 2024 • edited Loading

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024 via email

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024 • edited Loading

magne4000 commented Jun 27, 2024

magne4000 commented Jun 27, 2024 • edited Loading

magne4000 commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024 • edited Loading

brillout commented Jun 27, 2024

brillout commented Jun 27, 2024

brillout commented Jun 27, 2024

magne4000 commented Jun 27, 2024 • edited Loading

magne4000 commented Jun 27, 2024 • edited Loading

brillout commented Jun 28, 2024

brillout commented Jun 28, 2024

brillout commented Jun 28, 2024

magne4000 commented Jul 1, 2024

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024

brillout commented Jul 1, 2024

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024 • edited Loading

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024

brillout commented Jul 1, 2024

magne4000 commented Jul 1, 2024 • edited Loading

brillout commented Jul 1, 2024 • edited Loading

brillout commented Jul 3, 2024

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jun 27, 2024 •

edited

Loading

magne4000 commented Jul 1, 2024 •

edited

Loading

magne4000 commented Jul 1, 2024 •

edited

Loading

brillout commented Jul 1, 2024 •

edited

Loading