Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

JSX v4 record-based representation for lowercase #665

Merged
merged 4 commits into from Oct 8, 2022

Conversation

mununki
Copy link
Member

@mununki mununki commented Sep 29, 2022

This PR changes the internal representation of JSX v4 lowercase e.g. <div /> from object to record

@mununki
Copy link
Member Author

mununki commented Sep 29, 2022

@cristianoc What about the key for the props in lowercase? type JsxDOM.domProps has a key field as an optional there.

@mununki
Copy link
Member Author

mununki commented Sep 29, 2022

It seems fine in v4 sample project. More clean js output.

// as-is
function V4$Baz(props) {
  var name = props.name;
  var name$1 = name !== undefined ? name : "baz";
  var tmp = {};
  if (props.id2 !== undefined) {
    tmp.id = Caml_option.valFromOption(props.id2);
  }
  return React.createElement("div", tmp, name$1, props.children);
}

// to-be
function V4$Baz(props) {
  var name = props.name;
  var name$1 = name !== undefined ? name : "baz";
  return React.createElement("div", {
              id: props.id2
            }, name$1, props.children);
}

I'll run on the largest project.

@mununki
Copy link
Member Author

mununki commented Sep 30, 2022

I'll run on the largest project.

It works fine on my company project!

@cristianoc
Copy link
Contributor

@cristianoc What about the key for the props in lowercase? type JsxDOM.domProps has a key field as an optional there.

Seems OK to have key there: we know ahead of time it's one of the possible things to send.

@mununki
Copy link
Member Author

mununki commented Oct 1, 2022

I think I don't have anything left for this PR now.

@cristianoc
Copy link
Contributor

Ready to merge?

@cristianoc
Copy link
Contributor

How about updating the changelog?

@mununki
Copy link
Member Author

mununki commented Oct 8, 2022

How about updating the changelog?

Sure, I’ll.

@mununki
Copy link
Member Author

mununki commented Oct 8, 2022

How about updating the changelog?

I've added to Polish 4d4bb2c

@cristianoc
Copy link
Contributor

Not sure why suddenly CI gives a compile error on build

@mununki
Copy link
Member Author

mununki commented Oct 8, 2022

Let me rebase to the master

@mununki
Copy link
Member Author

mununki commented Oct 8, 2022

Build fixed 07190b4

@cristianoc cristianoc merged commit a40b8b6 into master Oct 8, 2022
@cristianoc cristianoc deleted the jsx-v4-lowercase branch October 8, 2022 18:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants