Inline props as object #66

davesnx · 2021-11-19T23:26:40Z

Fixes #43.

This PR changes the interface for React.Dom.domProps. Before these changes, domProps was a function with ~400 optional labelled arguments, after these changes we treat it as an object.

There are a few reasons for this change, but the most important is bundle size. jsoo compiles the optional arguments into zeros generating per each domProps call near 400 zeros, jsoo treated labeled arguments as positions into arrays. There are a few issues opened about this, I only found this one though: ocsigen/js_of_ocaml#801

Another of the reasons is about correctness, props in React are an object and after those changes, we treat it as an Object as well. Giving the same flexibility as the underneath layer could enable features that aren't possible on reason-react or rescript/react, for example adding missing attributes, allowing destructuring props in bindings, allowing "data"-attributes or type-safety on certain attributes.

Todos

Doubts

I have a doubt about the ref prop. Right now the ref is typed with React.Dom.Ref.t option but React.Dom.domRef doesn't seem to return an optional. But I believe the bindings are not the same as reason-react since they use an anonymous type from the useRef.

File "example/src/Code.re", line 14, characters 14-45:
14 |     <code ref={React.Dom.Ref.domRef(codeRef)}> {text |> React.string} </code>
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression has type React.Dom.domRef
       but an expression was expected of type React.Dom.domRef option

Future

As mentioned before, this change allows us a few niceties on the dom attributes API, from the top of my head:

-We could add more type-safety on many properties:
- polyvariants (for example ?preload is now string (* "none", "metadata" or "auto" (and "" as a synonym for "auto") *) into 'None | 'Metadata | 'Auto)
- dates (for example: max (* should be int or Js.Date.t *) into Js.Date
- regex
- etc
We could add warnings/errors about deprecated attributes
Try to implement "data" attributes (if the propName starts with "data", we render the value without any typecheck.

PS: I have the editor configured to format using ocamlformat, I believe we should run it across all files (I separate the formatting into one commit, in order to not affect the review)

jchavarri

@davesnx This is super cool. 😍

Re: the refs, there is some context about these decisions in reasonml/reason-react#421 (comment). Essentially forwardRef is unsafe in ReasonReact so I tried to fix that by introducing a new type for refs for dom elements (Dom.Ref.t).

After your changes, I just realized that we could keep the ref as non-optional, and actually it makes sense to do so, because in some cases (like when using useRef) we get a {current: elementOrNull} JS object, so converting back and forth was not trivial.

I updated the tests as well, and checked that the ref examples were working correctly.

Another thing, I checked the bundle size of the example (built with profile=prod) and we went from 200KB to 117KB for the file App.bc.js. 🎉 This is all application code (not react or prism libs), but I think it's a huge improvement.

Giving the same flexibility as the underneath layer could enable features that aren't possible on reason-react or rescript/react, for example adding missing attributes, allowing destructuring props in bindings, allowing "data"-attributes or type-safety on certain attributes.

This is SO exciting.

Thanks for working on this! 🤝

lib/dom.mli

jchavarri · 2021-11-20T08:16:33Z

lib/dom.mli

+
+module DangerouslySetInnerHTML : sig
+  (* Need to check the representation in JS of this type, can we have an Obj with a field type-safe in jsoo? *)
+  type t = { __html: string }


You can leave t abstract and use js.builder, e.g.

val make: __html:string -> t [@@js.builder]

jchavarri · 2021-11-20T08:26:52Z

ppx/html.ml

@@ -0,0 +1,584 @@
+type attributeType = String | Int | Float | Bool | Style | Ref | InnerHtml


Do you think it would make sense to move this whole module to dom.mli? It seems all of it is part of Dom 🤔

dom.mli is processed by gen_js_api, but it should be easy to move the html.ml contents to it. The way the gen_js_api works allows to stop the preprocessor from processing any code with [@@@js.stop] and start it with [@@@js.start]. It also allows to "send" some code to the resulting ml file with [@@@js.implem]. With these three primitives, we can do everything we need, e.g. if we call the submodule Prop, so we would end up with Dom.Prop:

(* stop gen_js_api preprocessor so that types below are not processed *) [@@@js.stop] module Prop: sig (* can hide here the types or values that don't need to be public. *) type attrType = String | Int | Float | Bool | Style | Ref | InnerHtml type eventType = ... ... val propTypeList: prop list end (* Start preprocessing again *) [@@@js.start] (* This should go in resulting `.ml` file created by gen_js_api *) [@@@js.implem module Prop = struct type attrType = String | Int | Float | Bool | Style | Ref | InnerHtml type eventType = ... ... let propTypeList = ... end ] (* Now the rest of the file can continue being processed *)

I moved out of dom.mli since those get exposed on purpose to the user, while the html.ml/i shouldn't be. If makes more sense to keep it inside Dom, we can make a module called... Private__ or somekind to express it better. WDYT?

Ah right, I didn't realize the html.ml file was in ppx folder. 👍

Maybe just inline this inside this module in ppx.ml then? So it's clear that is something internal to the ppx.

About the modlue naming, I would use module Dom for consistency with the other one, and with React.js docs. https://reactjs.org/docs/dom-elements.html, as HTML has different attributes than the props passed to these elements.

But these are all nits :)

jchavarri · 2021-11-20T08:35:46Z

ppx/ppx.ml

-                     (label, mapper.expr mapper expression)) )
+          (* Filtering out the props that don't have a label, not sure how's possible *)
+          let propsWithName =
+            List.filter (fun (name, _) -> getLabel name != "") nonEmptyProps


Why is this needed? Swallowing silently is suspicious 😬

Probably the last prop as unit? I'm not sure actually. Will look into it and not merge this until it's "discovered/fixed".

Made it more explicit, was the unit

ppx/ppx.ml

jchavarri · 2021-11-20T14:09:50Z

This is a quick analysis of bundle size impact of this PR using webpack-bundle-analyzer.

For each case, I ran:

make build-prod from project root folder
Then yarn webpack:production from examples folder (previously adding new BundleAnalyzerPlugin() to webpack plugis list.

This branch (84e7869):

Master branch (1ff2683):

davesnx · 2021-11-24T23:28:08Z

ppx/ppx.ml

          let makePropField (arg_label, value) =
+            let valueLoc = value.pexp_loc in
+            let argLabelLoc = unionLocation loc value.pexp_loc in


I append both locations to ensure the argLabel is part of the error.

Before

File "example/src/Events.re", line 49, characters 6-13: 49 | <button type_="submit" xxxx=0> {"submit dis" |> React.string} </button> ^^^^^^^ Error: prop 'xxxx' isn't a valid React prop

Passing the expression loc

49 | <button type_="submit" xxxx=0> {"submit dis" |> React.string} </button> ^ Error: prop 'xxxx' isn't a valid React prop

After (with unioning the locations)

File "example/src/Events.re", lines 1-49, characters 6-35: -1 | <button type_="submit" xxxx=0> {"submit dis" |> React.string} </button> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Error: prop 'xxxx' isn't a valid React prop

Hope it's clear. We could get fancier: with the length of the argLabel and minus the expr loc minus 1 (the =) maybe can point to the argLabel. Let me know if you think is worth it.

This is great! Another idea (for the future ™️ ) could be to print after the error a link to a dedicated page in https://ml-in-barcelona.github.io/jsoo-react/ that explains native elements, and lists the whole list of tags that are allowed (the list could be shared code with the ppx ✨ ).

One nice thing is that because all these checks happen at compile time we don't need to care about the errors or urls making bundle size larger.

I append both locations to ensure the argLabel is part of the error.

Would it be easier to pick the location from the parent ast node? So we avoid having to manipulate locations this way.

I mean, the Pexp_apply one:

That's way smarter!

Pushed a commit with this change, and appended a small refactor afterwards.

jchavarri · 2021-12-05T18:46:37Z

Bringing this in. Thanks again @davesnx !

davesnx added 6 commits November 19, 2021 23:03

Run ocamlformat on ppx.ml

d7b663c

feat: Treat React.Dom.props as an object

afa13f2

feat: Ensure types with constraint expressions

b117553

feat: Ensure types with constraint expressions for Ref and Style

58801fb

feat: Ensure types with Event types

dcfb025

test: Update snapshots

92af333

davesnx requested a review from jchavarri November 19, 2021 23:29

davesnx and others added 7 commits November 20, 2021 06:56

feat: Add dangerouslySetInnerHTML

edfb50b

refactor: Cleanup a bit makeValue and props

0bfca3d

fix: Remove --unlock-base in opam install

e4e1bf3

chore: Remove double quotes on CI

23fa519

fix refs

b1bb627

simplify test

d39684c

promote tests

84e7869

jchavarri approved these changes Nov 20, 2021

View reviewed changes

jchavarri added 2 commits November 20, 2021 17:21

Merge branch 'master' into Inline-props-as-object

09dbdea

make format

c1bc7c7

jchavarri mentioned this pull request Nov 21, 2021

RFC: Better support for OCaml syntax #67

Merged

jchavarri and others added 4 commits November 21, 2021 14:52

Merge branch 'master' into Inline-props-as-object

b78d47f

chore: Fix comment to represent labeled argument ~props

e6cd22a

fix: Load right location for missing prop error

b60a523

refactor: Make filtering out unit explicit

b1c82b6

davesnx commented Nov 24, 2021

View reviewed changes

davesnx added 2 commits November 25, 2021 00:32

test: Add dangerouslySetInnerHTML case

5449457

chore: Fix formatting on input.re

34140c5

davesnx mentioned this pull request Nov 25, 2021

Improve type-safety of string based props #69

Open

davesnx and others added 3 commits November 26, 2021 11:25

refactor: Use Pexp_apply loc for errors

abd442b

refactor: Remove death code on transformLowercaseCall

e23e5b4

Merge branch 'master' into Inline-props-as-object

8d3aff2

jchavarri added 4 commits December 5, 2021 18:00

promote tests

7ea60ef

fix dangerously... + add test

922031d

update dangerouslySetInnerHTML test + promote

8ae85c0

format

f353634

jchavarri merged commit 1703865 into master Dec 5, 2021

jchavarri deleted the Inline-props-as-object branch December 5, 2021 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline props as object #66

Inline props as object #66

davesnx commented Nov 19, 2021 •

edited

Loading

jchavarri left a comment

jchavarri Nov 20, 2021

jchavarri Nov 20, 2021

davesnx Nov 21, 2021

jchavarri Nov 21, 2021

jchavarri Nov 20, 2021

davesnx Nov 22, 2021

davesnx Nov 24, 2021

jchavarri commented Nov 20, 2021

davesnx Nov 24, 2021

davesnx Nov 24, 2021

jchavarri Nov 25, 2021

jchavarri Nov 25, 2021 •

edited

Loading

jchavarri Nov 25, 2021

davesnx Nov 26, 2021

jchavarri commented Dec 5, 2021

		@@ -0,0 +1,584 @@
		type attributeType = String \| Int \| Float \| Bool \| Style \| Ref \| InnerHtml

Inline props as object #66

Inline props as object #66

Conversation

davesnx commented Nov 19, 2021 • edited Loading

Todos

Doubts

Future

jchavarri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri commented Nov 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri Nov 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchavarri commented Dec 5, 2021

davesnx commented Nov 19, 2021 •

edited

Loading

jchavarri Nov 25, 2021 •

edited

Loading