search

purescript-react / purescript-react-basic-dom

https://pursuit.purescript.org/packages/purescript-react-basic-dom
Apache License 2.0
11 stars 18 forks source link

Add `renderToString` and `renderToStaticMarkup` #6

Closed maxdeviant closed 4 years ago

maxdeviant commented 4 years ago

This PR adds support for the following ReactDOMServer methods:

I opted not to add the stream-based methods at this time (mostly cause I don't feel like typing the streams), but I can do that if it is desired.

maxdeviant commented 4 years ago

I believe this PR resolves https://github.com/lumihq/purescript-react-basic/issues/63.

i-am-the-slime commented 4 years ago

2 hours ago? I wanted to upstream this just now:

const reactDom = require("react-dom/server")
exports.renderToString = reactDom.renderToString
foreign import renderToString ∷ JSX -> String

And I was wondering the same thing about effectfulness. I didn't model it that way out of laziness but I suppose it wouldn't really hurt either.

maxdeviant commented 4 years ago

You're right, it probably doesn't hurt to have an Effect, because these are most likely to be used at the top-level of a program.

But I'm more interested in whether or not it is correct to model them with or without an Effect, but perhaps there is no definitive consensus on that? :slightly_smiling_face:

megamaddu commented 4 years ago

Commented on it above as well.. I'm not really sure. It could throw, but that's not a normal path so maybe that doesn't matter.

megamaddu commented 4 years ago

It'd be great to have the streaming functions as well, but that introduces Node dependencies. Which reminds me, react-dom/server is kind of a different package from react-dom as well.. but maybe neither of those things matter because it's a separate module and bundlers will ignore it.

megamaddu commented 4 years ago

I've kind of been lazy about this whole thing because the server-side version of suspense is still up in the air and I was curious how that would change things. I'd guess it'd be through new functions though to preserve compatibility so it is more laziness than a real concern.

maxdeviant commented 4 years ago

@spicydonuts Are there any particular changes you'd like me to make to this PR?

FWIW, I think that introducing the streaming functions may fit better in a separate PR since, as you pointed out, it introduces Node dependencies.

megamaddu commented 4 years ago

That's fine with me

megamaddu commented 4 years ago

Thanks!