Remove `Array JSX` "children" argument #15

maddie927 · 2018-02-23T08:24:00Z

Also some other things:

no more key warnings for normal usage
React.Basic no longer depends on React.Basic.DOM
displayName
rename react to reactComponent
add new react which behaves more like React.Basic.DOM components
(simpler api for PureScript-only components by pre-applying
createElement)
React 16 support

Closes #14

Also some other things: - no more key warnings for normal usage - `React.Basic` no longer depends on `React.Basic.DOM` - displayName - rename `react` to `reactComponent` - add new `react` which behaves more like `React.Basic.DOM` components (simpler api for PureScript-only components by pre-applying `createElement`) Closes #14

maddie927 · 2018-02-23T08:27:29Z

Definitely changed more than I originally set out to, but a lot of the changes seemed to complement each other.

maddie927 · 2018-02-23T08:30:25Z

codegen/index.js

      -> JSX
-    ${e} = ${voids.indexOf(e) >= 0 ? 'createElementNoChildren' : 'createElement'} "${e}"
-`).forEach((x) => console.log(x.replace(/^\n\ {4}/, '').replace(/\n\ {4}/g, '\n')))
+    ${reserved.includes(e) ? `${e}_` : e} = createElement (unsafeCoerce "${e}")


The unsafeCoerce converts the DOM string into a ReactComponent, which they are as far as createElement is concerned. DOM components are weird that way.

paf31 · 2018-02-23T16:16:11Z

codegen/index.js

 };
+const reserved = ["module", "data", "type", "newtype", "class", "instance", "where", "derive", "if", "then", "else", "case", "of"];


Since we're using these as record labels, there's no need to escape keywords (at least in recent versions of PureScript).

They become function names though, like div. The one that was giving me problems was data. Apparently it's an HTML tag.

I didn't realize data was a html tag. None of the others listed in reserved are html tags are they? @paf31 Was data not causing a problem in the previous version of this script?

I assumed I got a newer version of react-html-attributes or something. It seemed safer to add everything to the reserved list than have to remember and go back later if type or module become elements.

paf31 · 2018-02-23T16:22:07Z

src/React/Basic.js

+var React = require("react");
+var KeyedContainer = React.Fragment || "div";
+
+// Class polyfill


The license might need updating if we include this.

I'll look into it

paf31 · 2018-02-23T16:22:32Z

src/React/Basic.js

+var KeyedContainer = React.Fragment || "div";
+
+// Class polyfill
+var __extends =


What's the difference between using this and react-create-class again?

Autobinding and mixins. If we do use create-react-class we need to add it as a dependency and import it. I'm not able to find it on Github either, and it's much larger than this polyfill. Also from the perspective of someone who's never seen this library before and the first lines of the readme ask for an extra dependency they don't expect to need, I think it's an unnecessary obstacle/complication.

paf31 · 2018-02-23T16:23:37Z

src/React/Basic.purs

@@ -32,25 +29,57 @@ foreign import react_
 -- | constructed using the helper functions provided by the `React.Basic.DOM`
 -- | module (and re-exported here).
 react


I find this API a little more confusing. If we're renaming things, I like reactComponent for creating a ReactComponent, but react should just go away IMO, and if we want a function for creating JSX directly from a spec (did you have a use case in mind?) then we can probably find a new name for that.

That function already exists (createElement). The only difference is whether you use createElement at the time of component definition (like the DOM module does) or in a render function. It's not necessary, but I do think it's more pleasant to use if you're writing lots of PS components.

paf31 · 2018-02-23T16:24:14Z

src/React/Basic.purs

    , receiveProps: mkEffFn3 receiveProps
    , render: mkFn3 render
    }

-foreign import component_ :: forall props. Fn2 (ReactComponent props) props JSX
+-- | SetState uses an update function to modify the current state.
+type SetState state fx = (state -> state) -> Eff (react :: ReactFX | fx) Unit


👍 for moving to state -> state. I think this could let us build more interesting abstraction on top, without adding dependencies and complexity here.

This is a good change. I was playing with this library this weekend and had state issues with the current approach because of react and batching. This solved the issue I ran into.

paf31 · 2018-02-23T16:25:11Z

src/React/Basic.js

+};
+
+exports.createElement_ = function(el, attrs) {
+  return React.createElement.apply(null, [el, attrs].concat(attrs.children));


attrs.children might be undefined, right?

Good point 👍

paf31 · 2018-02-23T16:25:32Z

src/React/Basic.purs

-component = runFn2 component_
+createElement = runFn2 createElement_
+
+foreign import keyed :: Array { key :: String, child :: JSX } -> JSX


What about using a homogeneous record for this?

It's much harder to construct in PS and slightly harder to destruct in JS. This is for dynamic lists, where you're mapping over some data to produce JSX.

I should add docs to this

paf31 · 2018-02-23T16:26:25Z

src/React/Basic/DOM.purs

@@ -7,7 +7,7 @@

 module React.Basic.DOM where

-import React.Basic.DOM.Internal (createElement, createElementNoChildren)
+import React.Basic (createElement)


This reverses the dependencies which means we can't reexport things. I'd really like to be able to use a single qualified import of React.Basic which is why I went with the internal module.

Also, I think it would be good to give a name to unsafeCoerce :: String -> ReactComponent and then to use that, since that is actually a safe coercion if you consider ReactComponent to mean "something I pass to createElement".

Good point, I'll add that function.

As for module dependencies, the convenience of reexporting locks this library into DOM-only React, when in reality the only the only DOM-specific code in this library is the DOM module. This structure allows alternate base components, like native (and native-web).

If the div component was actually defined as a component instead of React having special support for strings-as-components, they would be imported from react-dom, not react. We might also want to add purescript-react-basic-dom at some point with a React.Basic.DOM.Render module. This DOM module could live in either one since it doesn't actually depend on the react-dom library.

- Attach TypeScript license for use of polyfill - Remove `react` and rename `reactComponent` back to `react` - More general `key` implementations

paf31 · 2018-02-27T22:37:27Z

src/React/Basic.js

+    this.state = spec.initialState;
+    return this;
+  }
+  __extends(Component, React.Component);


What happens if you leave this out, out of curiosity?

I don't think it will work, but I can try later. It'd be the same as creating a class to use as a component in ES2015 without adding extends React.Component.

I tried this, which worked:

Component.prototype = Object.create(React.Component.prototype);

I almost went that route originally but I was worried there might be some other implementation detail of class I wasn't aware of. I'm fine with using that instead though until we find a problem.

paf31 · 2018-02-27T22:38:27Z

src/React/Basic.js

+};
+
+exports.createElementKeyed_ = function(el, key, attrs) {
+  var attrsClone = __shallowCopy(attrs) || {};


Can we not accept a record including a key property here?

Yes, but that seems harder to type in PS. Have an idea?

I was thinking of this:

createElementKeyed :: forall props . ReactComponent { | props } -> { key :: String | props } -> JSX createElementKeyed = runFn2 createElementKeyed_ foreign import createElementKeyed_ :: forall props. Fn2 (ReactComponent { | props }) { key :: String | props } JSX

Oh right, that makes sense. We'd also be able to just use createElement_ for its implementation as well.

Never mind, we need both implementations because of the warning about keys.

codedmart · 2018-03-05T15:37:33Z

I don't know that I have any major complaints with these changes, but I think my personal preference is the current api of element {props} [children]. That just may be my opinion though.

paf31 · 2018-03-06T22:07:07Z

README.md

-example :: R.ReactComponent ExampleProps
-example = R.react
+example :: ReactComponent ExampleProps
+example = reactComponent


This looks like it was changed back to react?

paf31

LGTM, thanks!

maddie927 requested a review from paf31 February 23, 2018 08:27

maddie927 commented Feb 23, 2018

View reviewed changes

paf31 reviewed Feb 23, 2018

View reviewed changes

maddie927 added 2 commits February 23, 2018 11:16

Cleanup based on PR feedback

47f086e

- Attach TypeScript license for use of polyfill - Remove `react` and rename `reactComponent` back to `react` - More general `key` implementations

Rebuild docs

a0742c7

paf31 reviewed Feb 27, 2018

View reviewed changes

Alter createElementKeyed API, remove class polyfill

7a77a30

Rename stringComponent to unsafeCreateDOMComponent

6662afc

paf31 reviewed Mar 6, 2018

View reviewed changes

paf31 approved these changes Mar 6, 2018

View reviewed changes

Fix readme

8e89c14

maddie927 merged commit e8ff932 into master Mar 6, 2018

maddie927 deleted the michael/no-child-arrays branch March 6, 2018 22:11

zudov mentioned this pull request Jul 31, 2018

Childish DOM contemplations #49

Closed

		};
		const reserved = ["module", "data", "type", "newtype", "class", "instance", "where", "derive", "if", "then", "else", "case", "of"];

Remove Array JSX "children" argument #15

Remove Array JSX "children" argument #15

Uh oh!

Conversation

maddie927 commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maddie927 commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codedmart Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paf31 Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codedmart commented Mar 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paf31 left a comment

Choose a reason for hiding this comment

Remove `Array JSX` "children" argument #15

Remove `Array JSX` "children" argument #15

maddie927 commented Feb 23, 2018 •

edited

Loading

maddie927 commented Feb 23, 2018 •

edited

Loading

codedmart Mar 5, 2018 •

edited

Loading

paf31 Feb 27, 2018 •

edited

Loading