Open milesfrain opened 4 years ago
Tracking a proposal to add this utility function to the React library in lumihq/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.
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.
Oh, ok. Yeah, go ahead and make that change!
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)?
I'd prefer it be fixed upstream.
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 https://github.com/JordanMartinez/purescript-cookbook/issues/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
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?
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.
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 🤷
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.
All index.html
files already contain a <div id="root"></div>
for the purpose of rendering the application.
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?
Add an empty div tag
https://reactjs.org/docs/add-react-to-a-website.html#step-1-add-a-dom-container-to-the-html
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>
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.
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.).
I'd be good with the Try PureScript change. That would simplify things and still make the code readable.
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
<div id="root"></div>
to frame.html
in the trypurescript
repo, or<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 😄 ).
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.
using the that's already in trypurescript, and replacing the
lines in this repo with a similarelement 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.
Ah. Okay.
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 :)
Closed by #294
Oh, wait, the other React recipes still need to be updated. But, this is now unblocked.
Oh, wait, the other React recipes still need to be updated. But, this is now unblocked.
https://github.com/JordanMartinez/purescript-cookbook/pull/298
Follow-up to #197
The existing code produces a warning in the dev console with this strategy:
Here's a warning-free option that's a little more verbose:
It would be most ideal to have a utility function that handles this common pattern:
Tracking a proposal to add this utility function to the React library in https://github.com/lumihq/purescript-react-basic-dom/issues/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: