Skip to content
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.

Sign up for GitHub

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

typed keys 2 #813

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

typed keys 2 #813

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb8ba82
typed keys 2
jchavarri Nov 3, 2023
28a7072
add jsxskeyed to the mix
jchavarri Nov 3, 2023
29fc9be
fix children.map and cloneelement
jchavarri Nov 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions ppx/reason_react_ppx.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ module Binding = struct
{ txt = Longident.Ldot (Lident "React", "array"); loc })
[ (nolabel, children) ]

let unsafeArray ~loc children =
Builder.pexp_apply ~loc
(Builder.pexp_ident ~loc
{ txt = Longident.Ldot (Lident "React", "unsafeArray"); loc })
[ (nolabel, children) ]

let componentLike ~loc props return =
Ptyp_constr
( { loc; txt = Ldot (Lident "React", "componentLike") },
Expand Down Expand Up @@ -448,7 +454,7 @@ let jsxExprAndChildren ~ident ~loc ~ctxt mapper ~keyProps children =
when list = [] ->
( Builder.pexp_ident ~loc { loc; txt = Ldot (ident, "jsxKeyed") },
Some (label, key),
Some (Binding.React.array ~loc children) )
Some (Binding.React.unsafeArray ~loc children) )
| Some (ListLiteral { pexp_desc = Pexp_array list }), [] when list = [] ->
( Builder.pexp_ident ~loc { loc; txt = Ldot (ident, "jsx") },
None,
Expand All @@ -458,13 +464,13 @@ let jsxExprAndChildren ~ident ~loc ~ctxt mapper ~keyProps children =
children *)
( Builder.pexp_ident ~loc { loc; txt = Ldot (ident, "jsxsKeyed") },
Some (label, key),
Some (Binding.React.array ~loc children) )
Some (Binding.React.unsafeArray ~loc children) )
| Some (ListLiteral children), [] ->
(* this is a hack to support react components that introspect into their
children *)
( Builder.pexp_ident ~loc { loc; txt = Ldot (ident, "jsxs") },
None,
Some (Binding.React.array ~loc children) )
Some (Binding.React.unsafeArray ~loc children) )
| None, (label, key) :: _ ->
( Builder.pexp_ident ~loc { loc; txt = Ldot (ident, "jsxKeyed") },
Some (label, key),
Expand Down
6 changes: 4 additions & 2 deletions src/React.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,14 +301,16 @@ module Event = {
};

type element;
type elementKeyed;
type componentLike('props, 'return) = 'props => 'return;
type component('props) = componentLike('props, element);

external null: element = "null";
external float: float => element = "%identity";
external int: int => element = "%identity";
external string: string => element = "%identity";
external array: array(element) => element = "%identity";
external array: array(elementKeyed) => element = "%identity";
external unsafeArray: array(element) => element = "%identity";

/* this function exists to prepare for making `component` abstract */
external component: componentLike('props, element) => component('props) =
Expand All @@ -328,7 +330,7 @@ external createElementVariadic:

[@mel.module "react/jsx-runtime"]
external jsxKeyed:
(component('props), 'props, ~key: string=?, unit) => element =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having element and elementKeyed as a completely different type was the main problem that @davesnx and I were running into. There are legit use-cases for supplying a key to an element that is outside of an array and to prevent that at this level would be wrong IMO.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are legit use-cases for supplying a key to an element that is outside of an array

Can you share some specific examples please? I could only find 1 example on the whole Ahrefs codebase, of a function that was generating some element, and it could be easily fixed.

to prevent that at this level would be wrong IMO.

To me it's more a line of tradeoffs between approaches than right or wrong. Adding a type variable to element has some implications as well in terms of error messages, type complexity, and general dev experience. It also will break more existing code. Some of the impact can be seen already in #812, where a lot more code has to be modified. I can imagine several 3rd party libs out there expect element to be a type without variables.

I would love to see the approach on #812 applied on Ahrefs monorepo so that we can have more information about how a migration would look like (both for the monorepo code and the opam libs) and how the errors will show with element('a). That way, we can compare both approaches in a more complete way?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you share some specific examples please? I could only find 1 example on the whole Ahrefs codebase, of a function that was generating some element, and it could be easily fixed.

The most common usages of key in React that I have used (outside of arrays) are:

  • to completely re-initialize a component which will clear the state, for example an <input> should be tied to a stateful value from its parent and reset if that state is changed.
  • help rendering performance for components with many children that are dynamic, for example if you have a component where it has static children that are rendered or not rendered based on some value and those children are the same element type (i.e. all <div>) then React might cause unnecessary re-renders when a components sibling changes.

I would love to see the approach on #812 applied on Ahrefs monorepo so that we can have more information about how a migration would look like (both for the monorepo code and the opam libs) and how the errors will show with element('a). That way, we can compare both approaches in a more complete way?

Yeah! This is a great call and I can try and whip something up for you!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jchavarri I've been hitting enough blockers to try and get a PoC going for you today that I'm going to take a little break and come back to it later. I did a few things on my fork of the PR as well if you want to check them out.

(component('props), 'props, ~key: string=?, unit) => elementKeyed =
"jsx";

[@mel.module "react/jsx-runtime"]
Expand Down
6 changes: 4 additions & 2 deletions src/React.rei
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
type element;
type elementKeyed;
type componentLike('props, 'return) = 'props => 'return;
type component('props) = componentLike('props, element);

external null: element = "null";
external float: float => element = "%identity";
external int: int => element = "%identity";
external string: string => element = "%identity";
external array: array(element) => element = "%identity";
external array: array(elementKeyed) => element = "%identity";
external unsafeArray: array(element) => element = "%identity";

/* this function exists to prepare for making `component` abstract */
external component: componentLike('props, element) => component('props) =
Expand All @@ -26,7 +28,7 @@ external createElementVariadic:

[@mel.module "react/jsx-runtime"]
external jsxKeyed:
(component('props), 'props, ~key: string=?, unit) => element =
(component('props), 'props, ~key: string=?, unit) => elementKeyed =
"jsx";

[@mel.module "react/jsx-runtime"]
Expand Down
2 changes: 1 addition & 1 deletion src/ReactDOM.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1535,7 +1535,7 @@ external createDOMElementVariadic:
"createElement";

[@mel.module "react/jsx-runtime"]
external jsxKeyed: (string, domProps, ~key: string=?, unit) => React.element =
external jsxKeyed: (string, domProps, ~key: string=?, unit) => React.elementKeyed =
"jsx";

[@mel.module "react/jsx-runtime"]
Expand Down
2 changes: 1 addition & 1 deletion src/ReactDOM.rei
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1536,7 +1536,7 @@ external createDOMElementVariadic:
"createElement";

[@mel.module "react/jsx-runtime"]
external jsxKeyed: (string, domProps, ~key: string=?, unit) => React.element =
external jsxKeyed: (string, domProps, ~key: string=?, unit) => React.elementKeyed =
"jsx";

[@mel.module "react/jsx-runtime"]
Expand Down
2 changes: 1 addition & 1 deletion test/Hooks__test.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ module DummyStatefulComponent = {
let make = (~initialValue=0, ()) => {
let (value, setValue) = React.useState(() => initialValue);

<button key="asdf" onClick={_ => setValue(value => value + 1)}>
<button onClick={_ => setValue(value => value + 1)}>
value->React.int
</button>;
};
Expand Down
18 changes: 12 additions & 6 deletions test/React__test.re
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,12 @@ describe("React", () => {
act(() => {
ReactDOM.Client.render(
root,
<React.Fragment key=?title>
<div> "Child"->React.string </div>
</React.Fragment>,
[|
<React.Fragment key=?title>
<div> "Child"->React.string </div>
</React.Fragment>,
|]
->React.array,
)
});

Expand All @@ -309,9 +312,12 @@ describe("React", () => {
};

let render = author =>
<div key={author.Author.name}>
<div> <img src={author.imageUrl} /> </div>
</div>;
[|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this exposes the downside you talked about in the description.

I think this is a pretty big limitation, as you can't write this anymore:

module Foo = {
  [@react.component]
  let make = (~id) => {
    <div key=id />;
  };
};

module Bar = {
  [@react.component]
  let make = () => {
    let ks = Array.init(10, string_of_int);
    <> {Array.map(ks, id => <Foo id />)} </>;
  };
};

Check the error:

379 |     <> {Array.map(ks, id => <Foo id />)} </>;
                                  ^^^^^^^^^^
Error: This expression has type < id : string > Js.t -> React.elementKeyed
       but an expression was expected of type
         < id : string > Js.t React.component =
           < id : string > Js.t -> React.element
       Type React.elementKeyed is not compatible with type React.element

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the keys have to be added directly in the elements that are part of the list, so the keys added to the div in Foo in your example are useless.

Check the console warnings from React in this JS snippet, adapted from yours:

https://codesandbox.io/s/modest-http-dspy7k

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compiles fine, and it'll guarantee the keys are passed correctly to React:

module Foo = {
  [@react.component]
  let make = () => {
    <div />;
  };
};

module Bar = {
  [@react.component]
  let make = () => {
    let ks = Array.init(10, string_of_int);
    <div> {Array.map(ks, id => <Foo key=id />)->React.array} </div>;
  };
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, key needs to be on the call-site: <Foo id /> -> <Foo id key="" />

<div key={author.Author.name}>
<div> <img src={author.imageUrl} /> </div>
</div>,
|]
->React.array;

act(() => {
ReactDOM.Client.render(
Expand Down
Loading