Skip to content

Several propositions #15

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

Open
starper opened this issue May 5, 2019 · 3 comments
Open

Several propositions #15

starper opened this issue May 5, 2019 · 3 comments

Comments

@starper
Copy link
Contributor

starper commented May 5, 2019

Here are some changes that I think could be beneficial for this library:

  1. Put client side and server side functions in separate modules - on js level there are react-dom and react-dom/server modules, I think it makes sense to have the same kind of separation on purescript level, too.
  2. Make renderToString and renderToStaticMarkup effectful - these functions can have side effects, here you can find an example (calling collect, then toString, then renderToString will result in css being an empty string)
  3. Switch to spago as a better alternative to psc-package.

Don't know about the last one, but first two are definitely breaking changes. What do you think about it? If you are ok with these changes, I will make a PR.

@ethul
Copy link
Collaborator

ethul commented May 5, 2019

Thanks for the proposals.

  1. I have been also thinking about this separation; however, I wonder if we really need separate packages purescript-react-dom and purescript-react-dom-server. Some of the server functions return Node streams. I don't know if we'd want to include purescript node dependencies unless the server functions are being used.

  2. Thanks for the example. Are you describing that calling renderToString causes the state of the ServerStyleSheets to change? What happens if you call collect and then toString without ever calling renderToString? Is there a different result than calling collect, renderToString, and then toString?

  3. I am not opposed to switching to spago. I admit I haven't use it though so I don't have a strong opinion either way about it. Would it make sense to keep the psc-package.json if we switch to spago?

@starper
Copy link
Contributor Author

starper commented May 5, 2019

At first I was talking about two modules in the same package, but now you got me thinking. Having both purescript-web-dom and purescript-node-streams as dependencies may look a bit weird. Not that it would break anything (I guess), if you use webpack it should throw away all the node stuff, but still. Maybe it, indeed, would be better to have two separate packages.

Yes, ServerStyleSheets's state is populated by renderToString (collect uses context provider under the hood), so without renderToString the state will be empty and toString will return an empty string.

Basically spago is something like psc-package + pulp, so it can do everything psc-package can. The only difference is that it uses spago.dhall and packages.dhall to describe dependencies, both of them, as far as I know, can be generated from psc-package.json. Currently, spago mostly shines if what you are writing is an app, if you are writing a library, there is no real difference which one to use, spago or psc-package. In the end you will still need bower and pulp for publishing, but hopefully not for long.

@starper
Copy link
Contributor Author

starper commented May 5, 2019

Oh, and the last thing that I forgot to mention - both render and hydrate take the third optional argument, which is a callback. I think it makes sense to have something like this:

render :: ReactElement -> Element -> Effect Unit -> Effect (Maybe ReactComponent)
render = ...

render' :: ReactElement -> Element -> Effect (Maybe ReactComponent)
render' x y = render x y $ pure unit

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

No branches or pull requests

2 participants