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

Consider renaming renderToStream to renderToNodeStream #10388

Closed
aickin opened this issue Aug 4, 2017 · 13 comments
Closed

Consider renaming renderToStream to renderToNodeStream #10388

aickin opened this issue Aug 4, 2017 · 13 comments

Comments

@aickin
Copy link
Contributor

aickin commented Aug 4, 2017

With the checkin of #10362, the code to render to a stream now looks like:

import { renderToStream } from 'react-dom/server'

renderToStream(<div>some text</div>).pipe(response);

This is great! I am, however, a little concerned about taking the word "node" out of the mix, because I'm thinking that we may eventually want to render to browser streams, which are a really neat feature that can be used in concert with "service-worker-side rendering". I would propose renaming renderToStream to renderToNodeStream and renderToStaticStream to renderToStaticNodeStream. This would give room for renderToBrowserStream in the future.

If I make a PR on this, would it have a chance of being accepted?

cc @gaearon & @sebmarkbage

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2017

I think we can do it.

@bjrmatos
Copy link
Contributor

bjrmatos commented Aug 4, 2017

maybe another option can be to export renderToStaticStream, renderToStream directly in react-dom -> import { renderToStream, renderToStaticStream } from 'react-dom' when browser streams are ready, and leaving the server part (node streams) in react-dom/server -> import { renderToStream, renderToStaticStream } from 'react-dom/server'

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2017

The thing I like about "node stream" naming is now people know to google "node streams" to find documentation about it. It wasn't obvious before what kind of API the stream provides.

@bjrmatos
Copy link
Contributor

bjrmatos commented Aug 4, 2017

good point, makes sense, i think explicitness wins in this case 😄

@aickin
Copy link
Contributor Author

aickin commented Aug 4, 2017

I think we can do it.

To be clear, do you mean you can accept such a PR, or you can do the name change yourselves? I'm fine either way, just want to understand if I should work on it or not. ;)

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2017

We'll take a PR :-)

@sebmarkbage
Copy link
Collaborator

Both environments have claimed the word Stream which is mostly fine because they're not expected to mix. So you just use one notion of the opaque Stream thing in one and get a different in the other.

One is more likely to leak into other environments though. Such as Electron or React Native apps. Which one are they going to favor? If it spreads then Node Stream is a bit of misnomer. (it's not "Browser Promise".) If it's considered legacy then it might make sense though.

@aickin
Copy link
Contributor Author

aickin commented Aug 4, 2017

Both environments have claimed the word Stream which is mostly fine because they're not expected to mix. So you just use one notion of the opaque Stream thing in one and get a different in the other.

I'm not sure what you mean. Are you suggesting that the same method would return two different types of objects depending on its environment? That seems counterintuitive to me, and sample code using the method would be very confusing, as it would only work in the right environment.

Such as Electron or React Native apps. Which one are they going to favor? If it spreads then Node Stream is a bit of misnomer.

I mean, it still came from Node. But Electron is an interesting example, as I believe it includes both Chromium and Node APIs, so both web streams and Node streams are available there, I think.

@aweary
Copy link
Contributor

aweary commented Aug 9, 2017

I think it's acceptable to risk a minor misnomer in this case. The target audience will understand that node streams refer to the stream API implemented and popularized by node, even if they're being utilized in other environments.

@sophiebits
Copy link
Collaborator

What do you think of renderToStreamNode instead? renderToNodeStream makes me wonder "what's a node stream? what else would be in the stream if not DOM node markup?"

@aweary
Copy link
Contributor

aweary commented Aug 9, 2017

renderToStreamNode makes me wonder if there's some kind of DOM node that supports streams 😄 I think there's potential for confusion, either way, so maybe it's worth asking the community what they think is clearer?

@aickin
Copy link
Contributor Author

aickin commented Aug 9, 2017

What do you think of renderToStreamNode instead?

I worry that it seems like "Stream" is an adjective modifying the noun "Node", and that the method would be expected to return a node. Sigh. We don't have enough words.

@aickin
Copy link
Contributor Author

aickin commented Aug 10, 2017

Fixed in #10425. Thanks!

@aickin aickin closed this as completed Aug 10, 2017
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

No branches or pull requests

6 participants