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

feat: add async option for streaming #352

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ncking
Copy link

@ncking ncking commented Jul 23, 2024

Description

Add async to options, so that the script will be executed as soon as it is available.
This allows our the client code to run ASAP, while the document is streaming.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@hi-ogawa
Copy link
Collaborator

MDN says async attribute doesn't work for inline script, so I'm not sure if this actually achieves what you're trying to do.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#async

I think I understand the intent, but it would be probably better if you can first demonstrate how current SSR streaming is inconvenient without this flag.

Actually I've been wondering about a similar issue when running transformIndexHtml to inject @vite/client and /@react-refresh then SSR-ing with bootstrapModules to inject main client entry script.
I noticed that neither @vite/client nor /@react-refresh inline script has async so I would assume those are blocked until browser loads all html. On the other hand, React add async to bootstrapModules, so those scripts try to run first.
This would likely to cause Can't detect preamble error, so I adjusted my client entry to wait for window.$RefreshReg$ to available https://github.com/hi-ogawa/vite-plugins/blob/9accac5755765d4a35dab5aceb3585efd9c163a0/packages/react-server/src/plugin/index.ts#L432-L439
If you happen to have a similar issue, then I would be happy to hear your setup.

@ArnaudBarre
Copy link
Member

For info I don't think adding complexity to speed up something that is dev only is a good thing. You will never be able to test prod streaming in a dev env. I'm not opposed to this, but we should have strong evidence that this required for some SSR/streaming setup to be added

@ncking
Copy link
Author

ncking commented Jul 30, 2024

MDN says async attribute doesn't work for inline script, so I'm not sure if this actually achieves what you're trying to do. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#async

I think I understand the intent, but it would be probably better if you can first demonstrate how current SSR streaming is inconvenient without this flag.

Actually I've been wondering about a similar issue when running transformIndexHtml to inject @vite/client and /@react-refresh then SSR-ing with bootstrapModules to inject main client entry script. I noticed that neither @vite/client nor /@react-refresh inline script has async so I would assume those are blocked until browser loads all html. On the other hand, React add async to bootstrapModules, so those scripts try to run first. This would likely to cause Can't detect preamble error, so I adjusted my client entry to wait for window.$RefreshReg$ to available https://github.com/hi-ogawa/vite-plugins/blob/9accac5755765d4a35dab5aceb3585efd9c163a0/packages/react-server/src/plugin/index.ts#L432-L439 If you happen to have a similar issue, then I would be happy to hear your setup.

It is a **module script & it outlines the behaviour for async module scripts in the "Attributes" section

For [module scripts](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules), if the async attribute is present then the scripts and all their dependencies will be fetched in parallel to parsing and evaluated as soon as they are available

& Im not "trying" ... It is working as per the spec/expected

@ncking
Copy link
Author

ncking commented Jul 30, 2024

For info I don't think adding complexity to speed up something that is dev only is a good thing. You will never be able to test prod streaming in a dev env. I'm not opposed to this, but we should have strong evidence that this required for some SSR/streaming setup to be added

This is not to "speed up" anything , it is a requirement for streaming to work as expected in DEV,
... Streaming in DEV is **exactly what I want ... ive been using in dev for months now ... it works perfectly & as expected . ..
just thought I should add via a PR
I dont understand " You will never be able to test prod streaming in a dev env" ... my code is working perfectly as per the spec & expected??

Im supprised at the level of negativity/misdirection, my simple PR has attracted ...

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jul 30, 2024

Im supprised at the level of negativity/misdirection, my simple PR has attracted ...

Sorry if you felt that way. I didn't mean that at all and thanks for the PR to bring this up. I didn't share previously but I had this PR on Remix in mind remix-run/remix#7842, which not only added async but also moved inline script to a dedicated src, so I thought inline script would be still an issue.
(Also note that Remix still had issues after remix-run/remix#7842 and eventually they moved their "react preamble" directly right next to client entry import so the execution order is guaranteed remix-run/remix#7919)

I'll check again later, but again it would be still helpful if you can provide a demo app to show the difference of this change.

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jul 31, 2024

I created a simple example here https://github.com/hi-ogawa/reproductions/tree/main/vite-react-streaming-preamble-race-condition-352 and my streaming server setup looks like this.

I added check-preamble.js as bootstrapModule to see if race condition is likely and indeed that can be observed, however <script async /> doesn't seem to change the situation either.

I recognize this as a general issue and the solution is desired, but I'm not sure what's the best way and whether it can/should be fixed on framework/integration side or vite plugin side.

Again It's totally possible that your have a ssr streaming setup totally different from what I have imagined, so I would appreciate if you can provide an example.

@hi-ogawa
Copy link
Collaborator

hi-ogawa commented Jul 31, 2024

Oh, wait, am I totally misdirecting? Looking back your description, it says about the execution of preamble and you are not actually concerned with preamble race condition with bootstrap scripts?

If so, I apologize and we would need more context on your wish of "script will be executed as soon as it is available" and if there's any critical reason that's desired.

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.

3 participants