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

Allow passing async function to injectToStream #40

Closed
nitedani opened this issue Jul 1, 2024 · 6 comments
Closed

Allow passing async function to injectToStream #40

nitedani opened this issue Jul 1, 2024 · 6 comments
Labels
enhancement ✨ New feature or request

Comments

@nitedani
Copy link
Collaborator

nitedani commented Jul 1, 2024

Example:

stream.injectToStream(async () => {
  const injectionsString = await something()
  return injectionsString
})

It would "block" the stream until the async function is resolved, while queuing other chunks in the meantime.
When the async function is resolved, it would write the resolved value to the stream, then continue with the queued chunks.

The following is not the same, the stream can close while we are waiting for something

const injectionsString = await something()
stream.injectToStream(injectionsString)

It would be nice for vikejs/vike-react#125, I think this is the only missing piece for the PR. It's working as-is, but can't send the last chunk for hydration, because the stream is closed.

With this implemented, I could mimic this await behavior: https://github.com/apollographql/apollo-client-nextjs/blob/e7a59cb26716a77bbfac659f435f89f5af8eff61/packages/client-react-streaming/src/stream-utils/createInjectionTransformStream.tsx#L104

Why await is needed in the linked code, is a different issue: apollographql/apollo-client-nextjs#325

@brillout brillout added the enhancement ✨ New feature or request label Jul 1, 2024
@brillout
Copy link
Owner

brillout commented Jul 1, 2024

This is actually something I've been wondering. As soon as the stream provided by React closes, the wrapper stream (created by react-streaming) is closed, causing a race condition when things are injected at the end of the stream.

Passing a promise or an async function to injectToStream() works only if injectToStream() is called before React finishes. Otherwise there is still a race condition.

I wonder if there is another (more reliable?) way to tell react-streaming to not close the wrapper stream too early?

brillout added a commit that referenced this issue Jul 1, 2024
@brillout
Copy link
Owner

brillout commented Jul 1, 2024

How about this:

const stream = useStream()
const makeClosableAgain = stream.doNotClose()
// ...
stream.injectToStream(someChunk)
// ...
makeClosableAgain()

@nitedani
Copy link
Collaborator Author

nitedani commented Jul 1, 2024

I think it's not good and unpredictable for my use case because it can send the chunk too late. We need to "pause" the stream and respect the order of injectToStream calls. apollographql/apollo-client-nextjs#325 (comment)

Example issue with this:
Call 1:

const stream = useStream()
const makeClosableAgain = stream.doNotClose()
const someChunk = await something() 
stream.injectToStream(someChunk)
makeClosableAgain()

Call 2:

const stream = useStream()
stream.injectToStream("something that depends on Call 1")

Output:
"something that depends on Call 1"
someChunk

Passing an async function allows us to pause the stream until it resolves and respect the order of injectToStream calls:
Call 1:

stream.injectToStream(async () => {
	const someChunk = await something() 
	return someChunk;
})

Call 2:

stream.injectToStream("something that depends on Call 1")

Output:
someChunk
"something that depends on Call 1"

Passing a promise or an async function to injectToStream() works only if injectToStream() is called before React finishes.

I think this is ok.

I think passing an async function and doNotClose have separate use cases, and both can be good for different things.

@brillout
Copy link
Owner

brillout commented Jul 1, 2024

Makes sense, let me see if I can implement chunks as async functions.

@brillout
Copy link
Owner

brillout commented Jul 3, 2024

Both implemented and released.

@brillout
Copy link
Owner

brillout commented Jul 3, 2024

You can now pass a chunk promise. It isn't exactly what you wanted (you cannot pass an async function) because I think passing chunk promises is more accurate, but let me know if it doesn't work out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants