-
-
Notifications
You must be signed in to change notification settings - Fork 8
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.
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
Handle DomProps with both jsx and html attribute name #171
Handle DomProps with both jsx and html attribute name #171
Conversation
d19dd3a
to
ae24de2
Compare
packages/react/src/DomProps.ml
Outdated
@@ -1659,7 +1659,14 @@ let camelcaseToKebabcase str = | |||
in | |||
str |> chars_of_string |> loop [] |> List.rev |> string_of_chars | |||
|
|||
let findByName tag jsxName = | |||
let findByName name = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where this is being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was used on getPropByName
for domProps
on ReactDom
. But removed due to this comment: #171 (comment)
packages/react/src/React.ml
Outdated
@@ -339,8 +339,8 @@ module JSX = struct | |||
| Inline of string | |||
|
|||
type prop = | |||
| Bool of (string * bool) | |||
| String of (string * string) | |||
| Bool of ((string * string) * bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use string * string * bool
instead, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to contextualize and have a better comprehension of what a prop name is (HTML and JSX) and its value. I left this comment on the description to another approach:
#171
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to keep a single tuple for perf, I haven't tested and just guessing but contextualisation here doesn't make much difference
packages/react/src/React.ml
Outdated
| Bool of (string * bool) | ||
| String of (string * string) | ||
| Bool of ((string * string) * bool) | ||
| String of ((string * string) * string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, not big reason to create a new tuple
packages/reactDom/src/ReactDOM.ml
Outdated
let getPropByName name = | ||
match DomProps.findByName name with | ||
| Ok prop -> (name, DomProps.getJSXName prop) | ||
| Error _ -> failwith "Invalid prop" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you move this into domprops, and remove the failwith?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was removed due to this comment: #171 (comment)
packages/reactDom/src/ReactDOM.ml
Outdated
let string name v = React.JSX.string (getPropByName name) v | ||
let int name v = React.JSX.int (getPropByName name) v | ||
let bool name v = React.JSX.bool (getPropByName name) v | ||
|
||
let booleanish_string name v = | ||
React.JSX.string (getPropByName name) (string_of_bool v) | ||
|
||
let float name v = React.JSX.float (getPropByName name) v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really don't want this, getPropByName is more expensive than the direct name, we always prefer to keep it static here
… into rsc * 'main' of github.com:ml-in-barcelona/server-reason-react: Handle DomProps with both jsx and html attribute name (#171) Change setSearch to handle SearchParams.t (#176) doc: add @@platform attribute documentation (#175) Support non-ascii on JS.String.toLowerCase and toUpperCase (#173) Fix snapshot test for external Upperbound reason to 3.10 feat: add nested mel.obj support (#169)
Description
Before, we had the HTML attribute for those
React.JSX.<type>
names, as the JSX isn't needed for SSR.However, we must have this JSX attribute name for the RSC component.
How
This PR adds to the DomProps the JSX attribute name data for
React.JSX.Bool
andReact.JSX.String
Instead of holding only the HTML attribute name,
React.JSX.Bool
andReact.JSX.String
now hold both the HTML attribute name and the JSX attribute.Discussion
@davesnx We talked about it to be a tuple, but could it be a record for a better read? What do you think?
How to test
make test-watch
into this branchmake demo
and see that nothing was changed