Compact and warning-free React recipes

[milesfrain]

Follow-up to #197

The existing code produces a warning in the dev console with this strategy:

main :: Effect Unit
main = do
  body <- body =<< document =<< window
  case body of
    Nothing -> throw "Could not find body."
    Just c -> do
      buttons <- mkButtons
      render (buttons {}) (HTMLElement.toElement c)

Here's a warning-free option that's a little more verbose:

-- App setup
main :: Effect Unit 
main = do
  root <- createRootElement
  buttons <- mkButtons
  render (buttons {}) root

-- DOM helper, creates an element we can mount
-- the React app into
createRootElement :: Effect Element
createRootElement = do
  doc <- document =<< window
  body <- maybe (throw "Could not find body element") pure =<< HTMLDocument.body doc
  root <- createElement "div" $ HTMLDocument.toDocument doc
  root # Element.setClassName "app-root"
  void $ appendChild (Element.toNode root) (HTMLElement.toNode body)
  pure root

It would be most ideal to have a utility function that handles this common pattern:

main :: Effect Unit 
main = renderInRoot $ mkButtons {}

Tracking a proposal to add this utility function to the React library in purescript-react/purescript-react-basic-dom#8

Also wondering if we should make code that's reused between recipes more generic (even if scripts/newRecipe.sh) handles this renaming for us. For example, removing "buttons" where possible:

myComponent :: Component {}
myComponent = do
  component "MyComponentName" \_ -> React.do
    -- button-specific code here

main :: Effect Unit 
main = renderInRoot $ myComponent {}

[JordanMartinez]

Tracking a proposal to add this utility function to the React library in purescript-react/purescript-react-basic-dom#8

I think it would be better to get this utility function merged and then update the cookbook here.

Also wondering if we should make code that's reused between recipes more generic (even if scripts/newRecipe.sh) handles this renaming for us. For example, removing "buttons" where possible:

It sounds like you are proposing a "shared" folder that contains code that all recipes can utilize if desired. If so, I'm against for two reasons. First, it could break the principle of "if it breaks the build, just move it into the 'broken' folder." If there is an issue in the shared code, it will affect all recipes. Second, it makes recipes less stand-alone where one can copy it and adjust things from there.
If that's not what you meant, please clarify.

[milesfrain]

Yes, the plan is to get the utility function into React first.

Not proposing shared cookbook code outside of recipes, but rather rewriting some of the react recipe code so that its identical between recipes.

So for example, remove "button" mentions from:

main :: Effect Unit
main = do
  body <- body =<< document =<< window
  case body of
    Nothing -> throw "Could not find body."
    Just b -> do
      buttons <- mkButtons
      render (buttons {}) (toElement b)

mkButtons :: Component {}
mkButtons = do
  component "Buttons" \_ -> React.do
    -- ...

To get:

main :: Effect Unit
main = do
  body <- body =<< document =<< window
  case body of
    Nothing -> throw "Could not find body."
    Just b -> do
      myComponent <- mkMyComponent
      render (myComponent {}) (toElement b)

mkMyComponent :: Component {}
mkMyComponent = do
  component "MyComponent" \_ -> React.do
    -- ...

This makes batch changes easier. #197 was a bit tedious with preserving these names.

[JordanMartinez]

Oh, ok. Yeah, go ahead and make that change!

[andys8]

My assumption is that most people will just have a div as container for their react application in HTML, since body is discouraged and results in a warning.

@JordanMartinez Is the idea still to push for a helper in react-basic-dom or should we go the cumbersome route and change the examples to a way that doesn't result in a warning (e.g. something like this)?

[JordanMartinez]

I'd prefer it be fixed upstream.

[andys8]

Hm, I'm not sure if (and when) the proposed helper function will make it into react basic. I doubt that it'll assume the existence of a div with id root in every application, too.

So what I'm thinking of is updating the current examples to render into a div#root instead of body like this. But initialization has to be adapter to createRoot anyway. So we could do this in one step with updating to react to v18, changing rendering, rendering to div and updating to PureScript 0.15.x.

When I've time I can give it a try. No sure if there are any blockers.

Update: I figured #291 is the current blocker.

diff --git a/recipes/HelloReactHooks/spago.dhall b/recipes/HelloReactHooks/spago.dhall
index 0046986..f1f0ea7 100644
--- a/recipes/HelloReactHooks/spago.dhall
+++ b/recipes/HelloReactHooks/spago.dhall
@@ -9,6 +9,7 @@
   , "react-basic-dom"
   , "react-basic-hooks"
   , "web-html"
+  , "web-dom"
   ]
 , packages = ../../packages.dhall
 , sources = [ "recipes/HelloReactHooks/src/**/*.purs" ]
diff --git a/recipes/HelloReactHooks/src/Main.purs b/recipes/HelloReactHooks/src/Main.purs
index ab5713f..e450ce0 100644
--- a/recipes/HelloReactHooks/src/Main.purs
+++ b/recipes/HelloReactHooks/src/Main.purs
@@ -1,25 +1,26 @@
 module HelloReactHooks.Main where
 
 import Prelude
+
 import Data.Maybe (Maybe(..))
 import Effect (Effect)
 import Effect.Exception (throw)
 import React.Basic.DOM (render)
 import React.Basic.DOM as R
 import React.Basic.Hooks (Component, component)
+import Web.DOM.NonElementParentNode (getElementById)
 import Web.HTML (window)
-import Web.HTML.HTMLDocument (body)
-import Web.HTML.HTMLElement (toElement)
+import Web.HTML.HTMLDocument (toNonElementParentNode)
 import Web.HTML.Window (document)
 
 main :: Effect Unit
 main = do
-  body <- body =<< document =<< window
-  case body of
-    Nothing -> throw "Could not find body."
+  root <- getElementById "root" <<< toNonElementParentNode =<< document =<< window
+  case root of
+    Nothing -> throw "Could not find root."
     Just b -> do
       helloComponent <- mkHelloComponent
-      render (helloComponent {}) (toElement b)
+      render (helloComponent {}) b
 
 mkHelloComponent :: Component {}
 mkHelloComponent = do

[JordanMartinez]

I'm not sure if (and when) the proposed helper function will make it into react basic.

Has this even been discussed in that repo yet?

[JordanMartinez]

I'm not sure if (and when) the proposed helper function will make it into react basic.

Has this even been discussed in that repo yet?

Just saw the link towards the start of this issue. Given that that's been open for a while, let's move forward with adding the helper function in each recipe. If react-basic-dom adds it later, another update can add it here. But I don't see why we should wait for them.

[pete-murphy]

I'm noticing there's a main tag in the Try PureScript iframe (https://github.com/purescript/trypurescript/blob/b5fec9bbb94cfb72e271fbdccbc4fbe4c8e92526/client/public/frame.html#L49), could we just target that in the recipes and avoid a createRootElement helper added to each recipe? I haven't checked if try.ps.ai has the same frame.html.

Edit: Ah, I'm realizing we'd also have to update all the <recipe>/web/index.html files to have a <main> as well, but that still doesn't seem like the worst solution to me 🤷

[JordanMartinez]

Ah, I'm realizing we'd also have to update all the /web/index.html files to have a

as well, but that still doesn't seem like the worst solution to me shrug

I'd be fine with that.

[andys8]

All index.html files already contain a <div id="root"></div> for the purpose of rendering the application.

https://github.com/jordanmartinez/purescript-cookbook/blob/f1ffd84b20197b7331f52a65e0f4b479cbecfa05/recipes/TicTacToeReactHooks/web/index.html#L9

The snippet in the first function instead creates a element and appends it to the body.

Since this repository can be taken as a starting point, I would try to keep the examples similar to what the most common way to initialize an application is. And it looks to me like this would use an element in the body and render into it (instead of body or other ways).

So, if there is no good reason to do it differently, I would recommend sticking to the initialization similar to how react docs suggest it.

Update: I'm wondering if this issue is more about try.purescript.org than about the code in the repo itself. Is this the case it wouldn't be easy to use existing html?

React docs

Add an empty div tag

https://reactjs.org/docs/add-react-to-a-website.html#step-1-add-a-dom-container-to-the-html

Create react app

npx create-react-app will create an html with a div and render into it.

npx create-react-app my-app
ack "id=\"root\""

public/index.html
31:    <div id="root"></div>

[pete-murphy]

Update: I'm wondering if this issue is more about try.purescript.org than about the code in the repo itself. Is this the case it wouldn't be easy to use existing html?

Yeah I think the issue with assuming div#root is that that's not the case in Try PureScript, but maybe we could add <div id="root"> to the frame.html in that repo. Although we currently have div#root (I believe) in all the React recipe HTML files in this repo, we're not using them and targeting <body> instead so that the recipes work in Try PureScript.

[andys8]

I think if we can use the <main id="main"></main> in try.purescript.org, it would be the best solution to target this one and change all index.html files to also contain main#main instead of div#root. The warning is not about the element tag or the id in use but to avoid only dynamically modifying the body (to avoid conflicts with scripts e.g. analytics.).

[JordanMartinez]

I'd be good with the Try PureScript change. That would simplify things and still make the code readable.

[pete-murphy]

I'd be good with the Try PureScript change. That would simplify things and still make the code readable.

Just to make sure I'm understanding, do you mean

1. adding <div id="root"></div> to frame.html in the trypurescript repo, or
2. using the <main id="main"></main> that's already in trypurescript, and replacing the <div id="root"></div> lines in this repo with a similar <main> element to target?

I don't really have a preference either way. As @andys8 noted, using <div id="root"> is typical in React apps, though not universal, and I could see the argument for just using <main id="main"> since that's what's in TPS already and wouldn't need to wait for changes to be approved for that repo (although looks like you might be a maintainer of that repo too 😄 ).

[JordanMartinez]

Ah, sorry for the ambiguity. I think pushing for 1 is desirable as I suspect that Try PureScript may be using main for other reasons that aren't necessary. However, I don't actually know.

[JordanMartinez]

using the

that's already in trypurescript, and replacing the

lines in this repo with a similar element to target?

The <main id="main"></main> element is what enables render $ Console.log "foo" to displays the string foo as an HTML element rather than logging that to the browser's console.

[andys8]

Ah. Okay.

https://github.com/purescript/trypurescript/blob/8919ccd6d7656c57286b9e1f23a6f3c00e4488ab/staging/src/TryPureScript.js#L3

But both render and withConsole are explicitly used in the trypurescript examples, but not in the cookbook, right?

This is probably going a bit too far, but on first glance it looks like TryPurecript.render should be the same behavior (rendering in div#root) whereas logging depends on if we want the replacement of the browser console to happen all the time or explicitly.

I would first get it working and then we can play around with combination of logging and rendering, and see if further changes would improve it :)

[JordanMartinez]

Closed by #294

[JordanMartinez]

Oh, wait, the other React recipes still need to be updated. But, this is now unblocked.

[andys8]

Oh, wait, the other React recipes still need to be updated. But, this is now unblocked.

#298