JSX PPX V4 part2 #586

mununki · 2022-06-27T12:01:13Z

cristianoc · 2022-06-27T17:06:21Z

After this #588, assuming all goes well, the change required will be simply to use @ns.optional instead of @optional which will be removed.

cristianoc · 2022-06-27T19:26:14Z

And this converts to @ns.optional including the JSX ppx: #589.
Would you take a look and see if you find any issues.

cristianoc · 2022-06-27T19:58:10Z

Would you rebase on master now that the surface syntax has landed.

cristianoc · 2022-06-28T01:32:38Z

Is the idea that clients of the ppx, such as rescript-react, will define their types usingJs.React....?
I guess we need to write a little spec for what those types are, so client libraries can use them.

mununki · 2022-06-28T01:58:22Z

Is the idea that clients of the ppx, such as rescript-react, will define their types usingJs.React....?

Frankly, I didn't think that far 😄, but it can be used by clients. Js.React is just for adding the new jsx transform bindings without breaking or updating the rescript-react at this stage. In order for type checker to check types without failure, Js.React needs to vendor the basic types such as Js.React.element, Js.React.domProps, etc. Especially, Js.React.domProps will be used to refactor v3 against type ReactDOMRe.domProps.

I guess we need to write a little spec for what those types are, so client libraries can use them.

Sure, I'll write down on it.

cristianoc · 2022-06-28T02:01:23Z

I thought this new jsx mode was opt-in, so off by default even on users of V4.
If true, this might in fact be the best situation to introduce a "breaking change" in that a newer version of rescript-react needs to be used.
Is this correct?

cristianoc · 2022-06-28T02:03:41Z

I think the new mode should have no reference to rescript-react at all. But the references should be the other way round: rescript-react (and others) can use the types defined in this PR (and the corresponding one in the compiler that adds types under Js.React).

Btw, instead of Js.React, perhaps Js.Jsx is better namespace.

cristianoc · 2022-06-28T02:15:18Z

Made some comments on the compiler PR, but realised it might be simpler to discuss directly on the spec once it is written.
So feel free to ignore those comments for now.

mununki · 2022-06-28T04:28:15Z

I've updated the spec in 75f8a28.

I thought this new jsx mode was opt-in, so off by default even on users of V4.
If true, this might in fact be the best situation to introduce a "breaking change" in that a newer version of rescript-react needs to be used.
Is this correct?

I agreed that the new jsx mode is better off by default even for the users of V4. I'm not clear about the direction of the core team regarding the rescript-react. I think it is the only choice to add Js.React and the bindings of the new jsx mode into the compiler if we want to keep the rescript-react from updating and breaking. Furthermore, it would be better design choice to opt in the jsx bindings and types. It is not a big deal in consideration that the jsx ppx is already sitting inside the compiler.

But, if the core team is wiling to update the rescipt-react and keep it as one of dependencies, then I can make PR for rescript-react for the new jsx mode. I think that releasing the V4 is the best chance to update the rescript-react.

cristianoc · 2022-06-28T06:50:13Z

I've updated the spec in 75f8a28.

I thought this new jsx mode was opt-in, so off by default even on users of V4.
If true, this might in fact be the best situation to introduce a "breaking change" in that a newer version of rescript-react needs to be used.
Is this correct?

I agreed that the new jsx mode is better off by default even for the users of V4. I'm not clear about the direction of the core team regarding the rescript-react. I think it is the only choice to add Js.React and the bindings of the new jsx mode into the compiler if we want to keep the rescript-react from updating and breaking. Furthermore, it would be better design choice to opt in the jsx bindings and types. It is not a big deal in consideration that the jsx ppx is already sitting inside the compiler.

But, if the core team is wiling to update the rescipt-react and keep it as one of dependencies, then I can make PR for rescript-react for the new jsx mode. I think that releasing the V4 is the best chance to update the rescript-react.

Definitely rescript-react can, and should be updated.
The only comment is: it must be possible for people to have a smooth upgrade path without updating everything. As they will have dependencies that they cannot upgrade as they're not the maintainers.
And they will have product code that they need to migrate gradually.
So I think a smooth upgrade path is one where the only thing you update is the V4 ppx. Everything still works. You convert little by little.
Once all is done, flip the switch and upgrade rescript-react.

Does it make sense?
I haven't specified what "flip the switch" means. It's open for design. One possibility would be: turn on the new jsx.

cristianoc · 2022-06-28T06:51:52Z

I think we need to explore the gradual upgrade path right now. As that affects the design of the various ppx options.
How does that sound?

cristianoc · 2022-06-28T06:56:12Z

About being React-specific: here's a TS page:
https://www.typescriptlang.org/docs/handbook/jsx.html

Notice in particular: "JSX is an embeddable XML-like syntax. It is meant to be transformed into valid JavaScript, though the semantics of that transformation are implementation-specific. JSX rose to popularity with the React framework, but has since seen other implementations as well"

I think it makes sense to aim for the same level of generality.

Notice it says "TypeScript ships with three JSX modes" and not "TypeScript ships with three React modes".

Hence my suggestion to use the Js.Jsx namespace not Js.React.

nkrkv · 2022-06-28T09:23:36Z

Btw, instead of Js.React, perhaps Js.Jsx is better namespace.

This! A language should outlast a framework. The community puts some effort trying to use ReScript with SolidJS, Remix, etc. And it will succeed one day I think. Hardcoding the “React” name into the language introduces an unnecessary and incorrect illusion that ReScript is only for React front-end apps.

mununki · 2022-06-28T12:40:39Z

I think we need to explore the gradual upgrade path right now. As that affects the design of the various ppx options. How does that sound?

Totally, it makes sense. I got your point. It is a good idea to explore the gradual upgrade path first, then fix the spec as the paths. Let me write some of the possible paths with the following specs.

mununki · 2022-06-28T13:30:04Z

I can’t agree you more about that the JSX doesn’t mean only for React, and I think it makes sense the namespace Js.Jsx instead of Js.React in some way.

But, here is my initial thought. If we use the namespace Js.Jsx here, it will contains all other frameworks’ JSX apis at the end. e.g. Js.Jsx.createElementForReact, Js.Jsx.createElementForSolid, Js.Jsx.createElementForRemix, etc.

We can add Js.Solid, Js.Remix after Js.React. (Maybe Jsx.React, Jsx.Solid could be another option. 😏 ) There will be the corresponding ppxes for each frameworks and be selected in ppx_entry.ml by user’s configuration. IMHO, it will be more flexible interface for the future major update of compiler.

I hope it conveys my thought correctly.

cristianoc · 2022-06-29T02:17:06Z

Perhaps one way to go is to do 2 of these, the easiest pair being React and React Native.
For React Native, one currently needs to install both rescript react, and rescript react native. A goal would be to be able to only install rescript react native, i.e. the part that adds bindings from actual components. I think for doing that, one needs to move the relevant types inside the compiler.

So one could, for example, have the core JSX types in Js.Jsx. Then both rescript react, and rescript react native, as independent packages, can make use of those types and add whatever is specific to the libraries.

This is just a though. By working out the details one can probably get a good idea.

I think one next step is to extent the spec with the types: what types do jsx and jsxDom have? One might be able to give them types that are independent from the specific framework. Or at least try, see if it makes sense.

cristianoc · 2022-06-29T02:23:38Z

cli/JSXV4UPGRADE.md

+
+   The `react.runtime` affects nothing, no matter of `"classic"` or `"automatic"`.
+
+   b. JSX V4 with classic mode


This doc looks good.
One aspect to consider more is how to adopt "jsx": 4 gradually. This bsconfig.json change is sufficient when the entire project is using V4. But we need to also think about what to do when half the project uses V3 and half the project uses V4.

At the very least one needs to specify per-file "jsx": 4 vs "jsx": 3 i.e. override the bsconfig locally.
In fact one might need to be able to do that in the middle of a file.
E.g. What if the file has<A > <B /> </A> and:
1 initially both A and B are in V3, and the current file is using V3
2 make the current file use V4
3 make A.res use V4
4 make B.res use V4

So at point 2, the jsx would cal <A> the V4 way but it expects to be called the V3 way.
I think this is solved by turning on V4 at the top of the file. And locally turning on V3 where <A > <B /> </A> is.

For 3, A uses V4 but B is still on V3.
We need to be able to turn on V3 inside <A > ... </A> so that we can call B with V3.

For 4, we don't need anything anymore. Just turn on V4 at the top of the current file.
When every file has been done like that and the project is converted, then non per-file config is needed anymore.

Does this make sense?
Is it going to work?

Just one minor comment. It's not "they". It is "us".
As long as people are kept in the loop and questions asked in the forum when in doubt, we can just go ahead make new version of rescript-react whenever needed.

It's you or Patrick's call, I can make PR for new Jsx module if you guys want. Or I'll await the Jsx module's interface are going to be fixed, once it is fixed, PPX V4 just emits to call that. I guess V4 is almost done now.

Definitely a PR for the new Jsx is useful for testing. Making sure that all works.
Then make a release when the time is right.

Definitely a PR for the new Jsx is useful for testing. Making sure that all works. Then make a release when the time is right.

Alright! I'm gonna make the Jsx module anyway in my local dev to test the V4. So, I'll make my proposal to rescript-react.

Side comment: Even though there was some confusion, it was a very fun journey for me. I learned a lot from you.

Likewise. It's a pleasure.

mununki · 2022-06-29T05:33:00Z

I think one next step is to extent the spec with the types: what types do jsx and jsxDom have? One might be able to give them types that are independent from the specific framework. Or at least try, see if it makes sense.

I think I got your point. I thought Js.React is only for V4 + new JSX mode ad-hoc workaround actually. I think I'm aligned to your intention now. I'll give it a try in that way.

cli/JSXV4UPGRADE.md

ryyppy · 2022-06-29T14:41:17Z

cli/JSXV4UPGRADE.md

+   }
+   ```
+
+   JSX V4 with `Js.React` which needs the peer-dependecy of React v17.\* or higher. It may break the project with dependencies which are using the explicit types of `rescript-react`.


Js.React

What's the idea of making it a submodule of Js? How is a user supposed to define this binding?
Also, do we again couple the JSX implementation to React? I thought we wanted to explore more liberate systems that will allow different targets.

It is just a test try to add missing bindings of the new JSX mode which is wrongly bound in rescript-react. #547 (comment)

I guess what I rather wanted to see is a config model that is similar to TS jsx option, also giving me the flexibility to define the binding module name / jsx factory function names of some sort.

I am sure there are lots of technical concerns that may not allow that, but I still wanted to mention it somewhere.

Oh, that is a good reference. Actually, runtime is referred to the babel configuration. https://babeljs.io/docs/en/babel-plugin-transform-react-jsx/#with-a-configuration-file-recommended

cli/JSXV4UPGRADE.md

cristianoc · 2022-06-30T15:49:40Z

I have update the upgrade docs based on the discussions. Let me know if it makes sense.

cli/JSXV4UPGRADE.md

mununki · 2022-06-30T15:55:44Z

I've added the proposal Jsx module in the rescript-react in my local project, and fixed the ppx to emit a call to it. It seems perfectly working. I'm fixing a small thing about the discouraged forwardRef example now. Once it is done, I'll make my proposal Jsx PR to rescript-react.

cristianoc · 2022-06-30T19:01:22Z

cli/reactjs_jsx_ppx_v4.ml

@@ -1064,7 +1064,8 @@ let transformComponentDefinition nestedModules mapper structure returnStructures
          @
          if hasForwardRef then
            [
-              (Location.mknoloc (Lident "ref"), Pat.var (Location.mknoloc "ref"));
+              ( Location.mknoloc (Lident "ref"),
+                Pat.var ~attrs:optionalAttr (Location.mknoloc "ref") );


Before continuing adding mknoloc that later need to be changed anyway.
The loc should be corresponding to something in the original source. The only question is whether the loc_ghost field is set to true or false.
(It would be set to true exactly once for each location used in the code produced by the ppx).

Good catch. I'm gonna look through the whole ppx using loc and polish the loc corresponding to original source code.

mununki · 2022-07-03T09:19:49Z

Updated the V4 upgrade doc as per rescript-lang/rescript@81029b3

mununki · 2022-07-18T15:38:43Z

Let me know when this is ready for another round of review. Meanwhile, master is now being used for compiler v10.1, so you will need to rebase on master. (Most importantly rebase is needed for the compiler repo which has new features merged; the syntax did not change much).

I made rebase to master. Actually, this PR is ready for another round of review.

cristianoc · 2022-07-18T15:46:06Z

Great. Anything specific the review should focus on?

mununki · 2022-07-18T15:53:30Z

Nothing special. I've focused several issues since jsx config update.

loc
forwardRef
more concrete props type

I think the forwardRef is less important than others, because it is discouraged usage anyway only for backward compatibility.

cristianoc · 2022-07-18T16:19:58Z

The way to test locations is to hook it up to the compiler, update the vendored parser in the extension, then build an example project with the compiler, turn on V4, and test the extension.
Hovering, jump to location etc.

mununki · 2022-07-18T16:39:36Z

I'm going to test it on my company project, and let you know. I've noticed one thing hovering information in the sample project I test on. The information from V3 looks more informative than V4 in terms of props type detail.

V3

V4

In my guess, it needs to be done in lsp server. Is there way to show the label declarations of props type in V4?

mununki · 2022-07-18T16:43:07Z

update the vendored parser in the extension

Hmm. I didn't acknowledge this. I'll try it.

cristianoc · 2022-07-18T18:39:56Z

I'm going to test it on my company project, and let you know. I've noticed one thing hovering information in the sample project I test on. The information from V3 looks more informative than V4 in terms of props type detail.

V3

V4

In my guess, it needs to be done in lsp server. Is there way to show the label declarations of props type in V4?

Yes there's the question of how best to represent the type information. Definitely the type definition would look better if it was expanded? Not sure it matters, but does the type definition generated by the ppx have dummy location or actual location info?

zth · 2022-07-18T18:46:25Z

Side note, and just vaguely related (read: please carry on, don't mind me) - it would be nice to "prettify" component hovers in general. TS also shows that in a very crude format, and it has always bothered me in both TS and ReScript. Maybe we could special case JSX component hovers somehow.

The exact format is ofc up for debate, but I'd much rather see something like this on a hover of a JSX component:

<Foo 
  name: string
  otherProp: int=?
/>

cristianoc · 2022-07-18T18:58:46Z

Side note, and just vaguely related (read: please carry on, don't mind me) - it would be nice to "prettify" component hovers in general. TS also shows that in a very crude format, and it has always bothered me in both TS and ReScript. Maybe we could special case JSX component hovers somehow.

The exact format is ofc up for debate, but I'd much rather see something like this on a hover of a JSX component:
<Foo 
  name: string
  otherProp: int=?
/>

That would happen for free if that was the way one writes the actual type definition.

cristianoc · 2022-07-18T19:02:20Z

To clarify: what if the syntax parser for types were extended. And the outcome printer.
One difficulty is that React.compnent is a pre-existing type, and its argument is locally, and invisibly, defined, nominal type.

cristianoc · 2022-07-18T19:04:55Z

In fact, anything that hover shows must be copy-pastable. So it follows that any form of clever hovering needs a syntax extension.

mununki · 2022-07-19T00:01:59Z

but does the type definition generated by the ppx have dummy location or actual location info?

type props which has generated by ppx has dummy loc.

mununki · 2022-07-19T00:14:59Z

I have a question, because I'm not deeply understanding how the extension and compiler works to show the hover. Is it different to show the type information by extension or maybe compiler between the object and record? I think there is difference between V3 and V4.

mununki · 2022-07-19T00:18:45Z

Can we merge and start a new PR for fine tunning? or we still can start a new PR which has base branch to this PR. The thread of this PR gets quite long to show at a glance without unfolding.

cristianoc · 2022-07-19T00:35:03Z

I think we can merge next.
First a question: have you verified that without changes to JSX settings the result of compilation of existing projects is identical?

We want to be reasonably confident that there are no breaking changes.

cristianoc · 2022-07-19T00:45:03Z

tests/ppx/react/expected/commentAtTop.res.txt

@@ -1,5 +1,6 @@
 type props<'msg> = {key?: string, msg: 'msg} // test React JSX file

+@react.component


Something to test here is that this works well e.g. with generating interface files with the vscode extension.

cristianoc · 2022-07-19T00:53:03Z

cli/reactjs_jsx_ppx.ml

+                rest ) ->
+            getPropTypes types rest
+          | Ptyp_arrow (Nolabel, _type, rest) ->
+            hasForwardRef := true;


Not sure I understand this.
How does one know that there is a forwardRef just because there's a function with a non-labelled arg which is not unit?

Is this check applying on the user code, or on code generated by the ppx?

Good catch. And it is quite a tricky part, because of the usage of forwardRef.
Basically, we are not allowing the nolabel arg in @react.component, right? There is one exception to this rule, forwardRef. So, for the interface, it has no way to know what the label is for the type signature. I think we need some assumptions and restrictions at some point. If there is a nolabel type signature after labeled args, the only ref can go there. It will be considered as forwardRef case.

When operating on a forwardRef, do we know that it's a forwardRef to begin with?
If that code was gated by a check "we are inside a forwardRef" then I would have no objection to it.
As it would be clear it's just a simple heuristic, and we don't care because it only applies to forwardRef.

So I imagine a reference insideForwardRef which is set. And checked at this point in the code.

cristianoc · 2022-07-19T00:53:35Z

OK made a few additional comments/questions.
Next I'll test this a bit.

cristianoc · 2022-07-19T01:02:50Z

Left a comment on rescript-lang/rescript#5484 to rebase. I'll proceed with testing after that.

mununki · 2022-07-19T02:22:18Z

First a question: have you verified that without changes to JSX settings the result of compilation of existing projects is identical?

We want to be reasonably confident that there are no breaking changes.

~~I've ran the compiler.alpah1 + V4 on my company repo, it has no diff in the generated js files! Nice. 🤞~~

EDIT: Sorry, false report. There are issues with compiler.alpha1 + V4.

mununki · 2022-07-19T02:53:27Z

EDIT: I was wrong. It was backward compatibility check with compiler.alpha1 + V3. 😓

There are some errors with compiler.alpha1 + V4. I'm going to leave it as an issue.

mununki · 2022-07-19T03:02:07Z

Here is the test bed https://github.com/green-labs/sinsunhi-frontend-mirror/tree/test-v4

cristianoc · 2022-07-19T03:56:50Z

Want to close this and continue on #614 ?

cristianoc · 2022-07-19T03:57:36Z

So backwards compatibility seems OK right?
The only question is debugging V4, correct? That's fine and expected.

mununki · 2022-07-19T04:15:34Z

So backwards compatibility seems OK right?

Yes, no diff and same output.

The only question is debugging V4, correct? That's fine and expected.

Yes. I'm back now, gonna leave an issue.

mununki · 2022-07-19T04:16:23Z

Want to close this and continue on #614 ?

Good! I'm gonna close this PR.

mununki force-pushed the jsx-ppx-v4-part2 branch 2 times, most recently from 1986d7a to 23c8915 Compare June 27, 2022 22:48

cristianoc reviewed Jun 29, 2022

View reviewed changes

ryyppy reviewed Jun 29, 2022

View reviewed changes

cli/JSXV4UPGRADE.md Outdated Show resolved Hide resolved

ryyppy reviewed Jun 29, 2022

View reviewed changes

cli/JSXV4UPGRADE.md Show resolved Hide resolved

mununki commented Jun 30, 2022

View reviewed changes

cli/JSXV4UPGRADE.md Show resolved Hide resolved

cristianoc reviewed Jun 30, 2022

View reviewed changes

mununki force-pushed the jsx-ppx-v4-part2 branch 2 times, most recently from 8863adf to a69d3c4 Compare July 3, 2022 09:04

mununki force-pushed the jsx-ppx-v4-part2 branch from c427c11 to 8c62db0 Compare July 18, 2022 15:36

cristianoc reviewed Jul 19, 2022

View reviewed changes

cristianoc mentioned this pull request Jul 19, 2022

Jsx ppx v4 part2 #614

Merged

mununki closed this Jul 19, 2022

mununki deleted the jsx-ppx-v4-part2 branch October 24, 2022 00:30


		The `react.runtime` affects nothing, no matter of `"classic"` or `"automatic"`.

		b. JSX V4 with classic mode

		@@ -1,5 +1,6 @@
		type props<'msg> = {key?: string, msg: 'msg} // test React JSX file

		@react.component

JSX PPX V4 part2 #586

JSX PPX V4 part2 #586

Conversation

mununki commented Jun 27, 2022 • edited Loading

cristianoc commented Jun 27, 2022

cristianoc commented Jun 27, 2022

cristianoc commented Jun 27, 2022

cristianoc commented Jun 28, 2022

mununki commented Jun 28, 2022

cristianoc commented Jun 28, 2022

cristianoc commented Jun 28, 2022

cristianoc commented Jun 28, 2022

mununki commented Jun 28, 2022 • edited Loading

cristianoc commented Jun 28, 2022

cristianoc commented Jun 28, 2022

cristianoc commented Jun 28, 2022

nkrkv commented Jun 28, 2022

mununki commented Jun 28, 2022

mununki commented Jun 28, 2022 • edited Loading

cristianoc commented Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

cristianoc Jun 29, 2022 • edited Loading

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

Choose a reason for hiding this comment

mununki commented Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

mununki Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

ryyppy Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianoc commented Jun 30, 2022

mununki commented Jun 30, 2022

Choose a reason for hiding this comment

mununki Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

mununki commented Jul 3, 2022

mununki commented Jul 18, 2022

cristianoc commented Jul 18, 2022

mununki commented Jul 18, 2022

cristianoc commented Jul 18, 2022

mununki commented Jul 18, 2022

mununki commented Jul 18, 2022

cristianoc commented Jul 18, 2022 • edited Loading

zth commented Jul 18, 2022

cristianoc commented Jul 18, 2022

cristianoc commented Jul 18, 2022 • edited Loading

cristianoc commented Jul 18, 2022

mununki commented Jul 19, 2022

mununki commented Jul 19, 2022

mununki commented Jul 19, 2022

cristianoc commented Jul 19, 2022

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

cristianoc commented Jul 19, 2022

cristianoc commented Jul 19, 2022

mununki commented Jul 19, 2022 • edited Loading

mununki commented Jul 19, 2022

mununki commented Jul 19, 2022

cristianoc commented Jul 19, 2022

cristianoc commented Jul 19, 2022

mununki commented Jul 19, 2022

mununki commented Jul 19, 2022

mununki commented Jun 27, 2022 •

edited

Loading

mununki commented Jun 28, 2022 •

edited

Loading

mununki commented Jun 28, 2022 •

edited

Loading

cristianoc commented Jun 29, 2022 •

edited

Loading

cristianoc Jun 29, 2022 •

edited

Loading

mununki commented Jun 29, 2022 •

edited

Loading

mununki Jun 29, 2022 •

edited

Loading

ryyppy Jun 29, 2022 •

edited

Loading

mununki Jul 1, 2022 •

edited

Loading

cristianoc commented Jul 18, 2022 •

edited

Loading

cristianoc commented Jul 18, 2022 •

edited

Loading

mununki commented Jul 19, 2022 •

edited

Loading