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

Don't force user to use pageContext.httpResponse.pipe() #38

Closed
brillout opened this issue Nov 30, 2023 · 9 comments · Fixed by #40
Closed

Don't force user to use pageContext.httpResponse.pipe() #38

brillout opened this issue Nov 30, 2023 · 9 comments · Fixed by #40

Comments

@brillout
Copy link
Member

See #37 (comment).

@brillout
Copy link
Member Author

@phonzammi Correct me if I'm wrong but, so far, I think the only solution is to have a new setting stream: boolean.

@phonzammi
Copy link
Member

phonzammi commented Nov 30, 2023

I think the only solution is to have a new setting stream: boolean.

I thought exactly the same.

Here are some usecases I've been thinking for a while (cmiiw):

if ssr: true then stream can be false or true

if ssr: false then stream must false

  1. set the stream setting manually:
    ssr is default to true,
    stream default to false.
    If users want to use httpResponse.pipe(), then they have to set stream: true, otherwise trow an error to return
    res.status(statusCode).send(body) or set stream: true.
    Pros : users have full control about streaming
    Cons (maybe) : users forced to set the stream setting

  2. set stream setting automatically
    If we can somehow find out what is returned from users server.js, (httpResponse.pipe() or res.status(statusCode).send(body)) then set stream to true or false automatically.
    Pros: users don't need to think about stream setting
    Cons: users don't have full control about streaming

And we also need to think about local config.h.ts, e.g /pages/posts/config.h.ts

Update: Just to mention, right now even if we set ssr to true or false in the config.h.ts, then in onRenderHtml() pageContext.config.ssr is undefined, is this intentional?

Any additional thought @AurelienLourot ?

@lourot
Copy link
Contributor

lourot commented Nov 30, 2023

Thanks a lot for looking into this @phonzammi

I would be for this:

  1. ssr, default is true
  2. stream, default is true, value is ignored when ssr is false

The way we expect most people to use vike-react is by scaffolding a project with https://batijs.github.io/ .Bati will make sure to scaffold a server code that works together with the vike-react options it selects. What do you think?

@brillout
Copy link
Member Author

One thought is that integrations such as vike-react-query and vike-react-telefunc will be able to set stream: true on behalf of the user.

@phonzammi
Copy link
Member

Wow @AurelienLourot , that makes sense, I never thought that.

@phonzammi
Copy link
Member

Update: Just to mention, right now even if we set ssr to true or false in the config.h.ts, then in onRenderHtml() pageContext.config.ssr is undefined, is this intentional?

@brillout & @AurelienLourot, should we concern about this? We can fix it alongside fixing this current stream setting

@lourot
Copy link
Contributor

lourot commented Dec 1, 2023

pageContext.config.ssr is undefined, is this intentional?

Yes, that's because that config is config-only (env: {config: true} used to be called env: 'config-only' in the past. I'd like to document env better in the future by the way, see vikejs/vike#1300), i.e. that config exists in the "config" environment, but not in the server and client environments.

This is enough, we don't need to have this config also in client environment (and bloat it) as there we can just know if SSR is enabled or not by looking at whether Page is defined or not

@phonzammi
Copy link
Member

Hi guys, may I try solve this issue? we only need to add setting in vike-react package right? I mean we don't need to touch vike's core right?

@brillout
Copy link
Member Author

brillout commented Dec 4, 2023

may I try solve this issue?

Certainly 💯

we only need to add setting in vike-react package right? I mean we don't need to touch vike's core right?

Yes.


On thing: I've a slight preference to set the setting's default value to false and let libraries such as vike-react-query set it to true. As always, feel free to disagree.

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

Successfully merging a pull request may close this issue.

3 participants