Merge type domProps and type props in JsxDOM #5706

mununki · 2022-09-29T15:08:18Z

This PR is to merge two props type for dom element into one. type JsxDOM.domProps and type JsxDOM.props has only difference which is ref type. The ref: domRef => unit is for the callback refs which was recommended in the earlier release of React 16.3. Furthermore, JSX ppx v3 and v4 don't use type props anymore.

cristianoc · 2022-09-29T16:35:20Z

When is this expected to land? In the next 10.1 release?
If so it should go the the 10.1 branch first. (Then cherry-picked later to master).

CHANGELOG.md

mununki · 2022-09-29T16:37:33Z

I expect that it depends on how the tests are going. Let me run on a larger project, then how about another round of test from the forum?

mununki · 2022-09-29T16:38:56Z

Should be ~~considered~~reviewed with rescript-lang/syntax#665

mununki · 2022-09-30T01:30:52Z

This will be the breaking change against the ReactDOM.domProps https://github.com/rescript-lang/rescript-compiler/blob/e8ccf3c1437882865b021bf65f12b380718ae825/jscomp/others/jsxDOM.ml#L288

method seems the reserved keyword in OCaml, anyhow avoiding the breaking?

cristianoc · 2022-09-30T04:30:03Z

This will be the breaking change against the ReactDOM.domProps

https://github.com/rescript-lang/rescript-compiler/blob/e8ccf3c1437882865b021bf65f12b380718ae825/jscomp/others/jsxDOM.ml#L288

method seems the reserved keyword in OCaml, anyhow avoiding the breaking?

Can you write this file in .res?

cristianoc · 2022-09-30T04:32:55Z

Also how about aria props, do they work with record representation? Better test them.

mununki · 2022-09-30T05:10:19Z

This will be the breaking change against the ReactDOM.domProps
https://github.com/rescript-lang/rescript-compiler/blob/e8ccf3c1437882865b021bf65f12b380718ae825/jscomp/others/jsxDOM.ml#L288

method seems the reserved keyword in OCaml, anyhow avoiding the breaking?

Can you write this file in .res?

💡💡💡

mununki · 2022-09-30T05:17:01Z

When is this expected to land? In the next 10.1 release?
If so it should go the the 10.1 branch first. (Then cherry-picked later to master).

I'll cherry-pick and make the base to ~~master~~ 10.1 branch.

mununki · 2022-09-30T11:55:10Z

Cherry-picked and rebased to 10.1_release

cristianoc · 2022-09-30T11:56:53Z

jscomp/others/jsxDOM.res

+  /* accessibility */
+  /* https://www.w3.org/TR/wai-aria-1.1/ */
+  /* https://accessibilityresources.org/<aria-tag> is a great resource for these */
+  /* [@optional] [@as "aria-current"] ariaCurrent: page|step|location|date|time|true|false, */


Want to update the stuff in comments too?

Sure, I will.
What was history left aria-* as comment?

Not sure why it is a comment.
And not sure why things are in multiple clusters of things where each cluster is in alphabetical order.

5b2554f uncomment.

cristianoc

Just a comment on comments.

cristianoc · 2022-09-30T12:20:30Z

There are a few more things that are keywords only in OCaml. Perhaps provide also the version without underscore?

begin
end
to

mununki · 2022-09-30T12:41:00Z

let _ = ReactDOM.createDOMElementVariadic("div", ~props={ariaCurrent: #page}, [])
// The field ariaCurrent does not belong to type JsxDOM.domProps

cristianoc · 2022-09-30T12:45:59Z

Something weird. I uncommented ariaCurrent?: [#page | #step | #location | #date | #time | #"true" | #"false"], then I ran on the example project, it builds failed.
let _ = ReactDOM.createDOMElementVariadic("div", ~props={ariaCurrent: #page}, [])
// The field ariaCurrent does not belong to type JsxDOM.domProps

"make lib" locally

mununki · 2022-09-30T13:03:16Z

"make clean" -> "npm install" -> "make lib"

a7ae475 I guess that JsxDOM.js should not suppose to be removed.
I have to go now, I'll be back in a couple of hours. I'll retry it.

cristianoc · 2022-09-30T13:38:27Z

@mattdamon108 there's a problem. Added commit to also build .res files in others/. The problem is that build dependencies are determined using ocamldep, which only works for .ml files. This means that jsxDom.res will have build errors.

scripts/ninja.js

mununki · 2022-09-30T13:39:46Z

@mattdamon108 there's a problem. Added commit to also build .res files in others/. The problem is that build dependencies are determined using ocamldep, which only works for .ml files. This means that jsxDom.res will have build errors.

Should I revert to .ml back?

cristianoc · 2022-09-30T13:48:51Z

@mattdamon108 there's a problem. Added commit to also build .res files in others/. The problem is that build dependencies are determined using ocamldep, which only works for .ml files. This means that jsxDom.res will have build errors.

Should I revert to .ml back?

Probably.
The alternative would be to hack ad-hoc the build system by making that file build last. But then the next change is going to be hairy.
Or find some other build method, which is going to take a while.
Or, move this outside the compiler into rescript-react.

cristianoc · 2022-09-30T13:51:50Z

Or, to make jsxDom.res self-contained file that does not depend on anything. Is it possible?

mununki · 2022-09-30T13:53:00Z

@mattdamon108 there's a problem. Added commit to also build .res files in others/. The problem is that build dependencies are determined using ocamldep, which only works for .ml files. This means that jsxDom.res will have build errors.

Should I revert to .ml back?

Probably. The alternative would be to hack ad-hoc the build system by making that file build last. But then the next change is going to be hairy. Or find some other build method, which is going to take a while. Or, move this outside the compiler into rescript-react.

Agreed. How about making just method_ to breaking change? I think JsxDOM locationg in the compiler would be better choice with Jsx.element

cristianoc · 2022-09-30T13:53:25Z

E.g. do we really need JsxEvent to be in a separate file?

mununki · 2022-09-30T13:55:37Z

E.g. do we really need JsxEvent to be in a separate file?

How about Jsx? Do you mean all the Jsx* togther?

cristianoc · 2022-09-30T13:56:49Z

E.g. do we really need JsxEvent to be in a separate file?

How about Jsx? Do you mean all the Jsx* togther?

Probably. As build dependencies both ways are missing.

cristianoc · 2022-09-30T13:57:46Z

E.g. do we really need JsxEvent to be in a separate file?

How about Jsx? Do you mean all the Jsx* togther?

Probably. As build dependencies both ways are missing.

Btw I'm not sure it's a good idea, just asking if possible at all.

cristianoc · 2022-09-30T14:04:01Z

Probably best to go back to .ml for now.

Longer term: an observation.
A bunch of files in tests build just fine, even thought that are .res. Some of them need to have zero dependencies, and some of them (such as in build tests) do not.
So it's conceivable to build certain things later, as long as there's nothing using them earlier on in the compiler.

mununki · 2022-09-30T14:05:35Z

Probably best to go back to .ml for now.

Longer term: an observation. A bunch of files in tests build just fine, even thought that are .res. Some of them need to have zero dependencies, and some of them (such as in build tests) do not. So it's conceivable to build certain things later, as long as there's nothing using them earlier on in the compiler.

Totally agreed. I'll revert JsxDOM.res to ml back.

mununki · 2022-09-30T14:07:24Z

Can I reset to the commit JsxDOM was ml instead of revert, if you don't mind?

cknitt · 2022-09-30T15:02:22Z

.res would be very nice to have because of the enums for the missing aria attributes.
Shall we create an issue to revisit that later?

mununki · 2022-09-30T15:03:31Z

.res would be very nice to have because of the enums for the missing aria attributes. Shall we create an issue to revisit that later?

That would be great!

cknitt · 2022-09-30T15:09:39Z

Added #5708 for tracking.

mununki · 2022-09-30T15:19:36Z

syntax JSX v4 record-based representation for lowercase syntax#665
rescript-react Deprecate ReactDOM.props & compat to JSX v4 lowercase rescript-react#63

With all changes in PRs, I ran on my company project. It works fine.

jscomp/others/jsxDOM.ml

cristianoc

This is probably good to go.
Is there a way to make git diff less confused? The diff shows domProps changed a lot while it has not. Mostly just removal.

mununki · 2022-10-01T05:23:58Z

Explored many paths and tried, but finally, ended up removing JsxDOM.props only.
Clean up the commits and rebased.

cristianoc reviewed Sep 29, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mununki force-pushed the jsx-v4-lowercase branch from 92fea47 to ebfc870 Compare September 30, 2022 11:54

mununki changed the base branch from master to 10.1_release September 30, 2022 11:54

cristianoc reviewed Sep 30, 2022

View reviewed changes

cristianoc approved these changes Sep 30, 2022

View reviewed changes

mununki commented Sep 30, 2022

View reviewed changes

scripts/ninja.js Outdated Show resolved Hide resolved

mununki force-pushed the jsx-v4-lowercase branch from 6008633 to 1d5cac8 Compare September 30, 2022 15:01

cknitt mentioned this pull request Sep 30, 2022

Convert JsxDOM.ml to JsxDOM.res and add missing aria attributes #5708

Closed

mununki mentioned this pull request Sep 30, 2022

Props types cleanup rescript-lang/rescript-react#61

Closed

cknitt reviewed Sep 30, 2022

View reviewed changes

jscomp/others/jsxDOM.ml Outdated Show resolved Hide resolved

cristianoc reviewed Oct 1, 2022

View reviewed changes

remove JsxDOM.props

684c4d4

mununki force-pushed the jsx-v4-lowercase branch from d0b4399 to 684c4d4 Compare October 1, 2022 05:22

cknitt approved these changes Oct 1, 2022

View reviewed changes

cristianoc merged commit 7dbcc93 into 10.1_release Oct 1, 2022

mununki deleted the jsx-v4-lowercase branch October 1, 2022 05:55

Merge type domProps and type props in JsxDOM #5706

Merge type domProps and type props in JsxDOM #5706

Conversation

mununki commented Sep 29, 2022

cristianoc commented Sep 29, 2022

mununki commented Sep 29, 2022

mununki commented Sep 29, 2022 • edited Loading

mununki commented Sep 30, 2022

cristianoc commented Sep 30, 2022

cristianoc commented Sep 30, 2022 • edited Loading

mununki commented Sep 30, 2022

mununki commented Sep 30, 2022 • edited Loading

mununki commented Sep 30, 2022

cristianoc Sep 30, 2022

Choose a reason for hiding this comment

mununki Sep 30, 2022

Choose a reason for hiding this comment

cristianoc Sep 30, 2022

Choose a reason for hiding this comment

mununki Sep 30, 2022

Choose a reason for hiding this comment

mununki Sep 30, 2022

Choose a reason for hiding this comment

cristianoc left a comment

Choose a reason for hiding this comment

cristianoc commented Sep 30, 2022

mununki commented Sep 30, 2022 • edited Loading

cristianoc commented Sep 30, 2022

mununki commented Sep 30, 2022

cristianoc commented Sep 30, 2022

mununki commented Sep 30, 2022

cristianoc commented Sep 30, 2022

cristianoc commented Sep 30, 2022

mununki commented Sep 30, 2022

cristianoc commented Sep 30, 2022

mununki commented Sep 30, 2022

cristianoc commented Sep 30, 2022

cristianoc commented Sep 30, 2022

cristianoc commented Sep 30, 2022

mununki commented Sep 30, 2022

mununki commented Sep 30, 2022 • edited Loading

cknitt commented Sep 30, 2022

mununki commented Sep 30, 2022

cknitt commented Sep 30, 2022

mununki commented Sep 30, 2022

cristianoc left a comment

Choose a reason for hiding this comment

mununki commented Oct 1, 2022

mununki commented Sep 29, 2022 •

edited

Loading

cristianoc commented Sep 30, 2022 •

edited

Loading

mununki commented Sep 30, 2022 •

edited

Loading

mununki commented Sep 30, 2022 •

edited

Loading

mununki commented Sep 30, 2022 •

edited

Loading