Skip to content

Conversation

@maxdeviant
Copy link
Contributor

This PR adds support for the following ReactDOMServer methods:

  • renderToString
  • renderToStaticMarkup

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
Copy link
Contributor Author

I believe this PR resolves purescript-react/purescript-react-basic#63.

@i-am-the-slime
Copy link
Member

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
Copy link
Contributor Author

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? 🙂

@maddie927
Copy link
Member

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.

@maddie927
Copy link
Member

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.

@maddie927
Copy link
Member

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
Copy link
Contributor Author

@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.

@maddie927
Copy link
Member

That's fine with me

@maddie927 maddie927 merged commit 2b75bc1 into purescript-react:main Aug 4, 2020
@maddie927
Copy link
Member

Thanks!

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