Skip to content

JSX V4: investigate leftovers after release. #5505

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

Closed
9 of 11 tasks
cristianoc opened this issue Jul 4, 2022 · 32 comments
Closed
9 of 11 tasks

JSX V4: investigate leftovers after release. #5505

cristianoc opened this issue Jul 4, 2022 · 32 comments
Milestone

Comments

@cristianoc
Copy link
Collaborator

cristianoc commented Jul 4, 2022

This is done, leaving leftovers for a future release.

This task is to track the various activities related to JSX V4.

@cristianoc cristianoc added this to the v10.1 milestone Jul 4, 2022
@cristianoc
Copy link
Collaborator Author

@mattdamon108 this topic has grown to be quite substantial. Adding a top-level issue to track the various activities taking place, to coordinate, and so that interested people can follow along.

@cristianoc
Copy link
Collaborator Author

@mattdamon108 I'm questioning the defaults.
Not sure the default should be "do nothing" as in V3. This is likely only confuse people.

How about making v4 react classic the default?

@mununki
Copy link
Member

mununki commented Jul 25, 2022

Is it what you want at the beginning? (backward compatibility) 😄 Switching the V4 as default is an effortless task like a finger snap.
One thing came to mind is the new jsx config in bsconfig. Switching the V4 as default without the hand-written configuration by the user, I'm afraid it becomes more confusing to our user. I think it looks mismatch between configuration and compilation. What do you think?

@cristianoc
Copy link
Collaborator Author

I think the vast majority of users will never need to change config. And beginners have 1 less thing to learn.
Plus there was a forum post right now about an issue due to not setting the configuration, which this would have prevented.

@cristianoc
Copy link
Collaborator Author

Also, there's no setting for "do nothing". So this is a weird initialization.

@mununki
Copy link
Member

mununki commented Jul 25, 2022

I think there is a setting for "do nothing", not adding "reason": { ... } nor "jsx": { ... }. If user doesn't add any of them, jsx ppx will not be triggered.

@mununki
Copy link
Member

mununki commented Jul 25, 2022

How about throwing more informative error message, in case user didn't add the jsx configuration, but has written JSX expressions or attribute @react.componet?

@mununki
Copy link
Member

mununki commented Jul 25, 2022

Before further discussion, let me clear your intention.

  1. no configuration at all -> V4 + classic
  2. configuration -> works as configuration

Is it correct?

@cristianoc
Copy link
Collaborator Author

More messages is OK. Except if it's an error it's a breaking change.

@cristianoc
Copy link
Collaborator Author

I think there is a setting for "do nothing", not adding "reason": { ... } nor "jsx": { ... }. If user doesn't add any of them, jsx ppx will not be triggered.

You can't do that per-file.
It's only a config that exists by omitting global config.

@mununki
Copy link
Member

mununki commented Jul 25, 2022

I got your point. Let me write down some user stories to make it clear.

First, "reason": 3 + V3 users -> JSX V4 + classic as default
If user has error or want to back to V3, then user changes the bsconfig "jsx": { "version": 3 }
If user wants to try V4 + automatic, go for it with bsconfig update, "jsx": {"version": 4, "mode": "automatic"}

In this regard, only concern is V4 + classic will be triggered even for users who doesn't have any configuration about jsx. I think it is not a big deal. That is how the ppx works normally, regardless of using an attribute or extension which trigger.
Does it make sense?

@mununki
Copy link
Member

mununki commented Jul 25, 2022

Yes, I think making V4 + classic as default brings the new feature with impact.

@mununki
Copy link
Member

mununki commented Jul 25, 2022

Oh, we can still make not triggering the jsx ppx if user doesn't have any configuration about jsx at all.

@mununki
Copy link
Member

mununki commented Jul 26, 2022

If we make V4 + classic as default, then submodules in dependencies will be affected by the same default, right? Is it your intention?

@mununki
Copy link
Member

mununki commented Jul 26, 2022

It is very clever idea! If we update some react component such as Context, Fragment with @react.component then everything will work accordingly! Same to other submodules.

Anyway, some components in this file https://github.com/rescript-lang/rescript-react/pull/49/files#diff-dc0e4227d38d3757b4c1cc61762295e29c7cb769e3644637d7769ea744c9f4d3 , needs to be updated though.

Plus, I'd like to change this PR, and change my initial suggestion. It is better to update React.res with jsx ppx change compatible with @react.component not making RescriptReact.res. (It causes more confusion I think) and It is better to guide users to install rescript-react per their purpose. If they want to use V3, then install pinned version 0.10.* something like that.

@cristianoc
Copy link
Collaborator Author

cristianoc commented Aug 26, 2022

@mattdamon108 now that 10.0 is out, time to think about 10.1 and JSX v4.

What is the current state? Are there parts still in progress or is it already time for detailed documentation?

@mununki
Copy link
Member

mununki commented Aug 26, 2022

Beside the decision about V4 as default, there is no stuffs left in progress.
Can you guide me how to fill the detailed documentation?

@cristianoc
Copy link
Collaborator Author

There's a spec which is mostly for implementers, but occasionally could be used by users.
I would make sure that it is up to date and clear enough.
Then, the changelog needs to be updated with a quick user guide: what are JSX modes, what they do, and how to set them. It's OK to refer to the spec for details. Especially breaking changes.
One that is done, one can invite people on the forum to have a go and report feedback.

@mununki
Copy link
Member

mununki commented Aug 26, 2022

One thing that came to my mind is to check if the user tries to use async with jsx definition as below. I'll make a PR if there is something to improve the error msg or etc.

@react.component
let make = async () => body

@mununki
Copy link
Member

mununki commented Aug 31, 2022

@cristianoc I found one thing left to be done. This PR needs to be done rescript-lang/rescript-react#49 which contains the new jsx transform api bindings. The ppx v4 is using it.

@cristianoc
Copy link
Collaborator Author

@cristianoc I found one thing left to be done. This PR needs to be done rescript-lang/rescript-react#49 which contains the new jsx transform api bindings. The ppx v4 is using it.

That is with automatic mode right? The spec should be a bit more explicit about what automatic does, and what are the possible parameters for "mode".

@mununki
Copy link
Member

mununki commented Aug 31, 2022

That is with automatic mode right?

Yes, correct.

@mununki
Copy link
Member

mununki commented Sep 1, 2022

That is with automatic mode right?

Yes, correct.

One more thing, https://github.com/rescript-lang/rescript-react/blob/8f31f375bc8965dad4a1c498b6ab69227f150069/src/RescriptReact.res#L1

RescriptReact module needs to be introduced with jsxConfig({version:r 4}) even though without automatic mode. Because V4 is not a default, React.res module will be transformed by V3.

@cristianoc
Copy link
Collaborator Author

That is with automatic mode right?

Yes, correct.

One more thing, https://github.com/rescript-lang/rescript-react/blob/8f31f375bc8965dad4a1c498b6ab69227f150069/src/RescriptReact.res#L1

RescriptReact module needs to be introduced with jsxConfig({version:r 4}) even though without automatic mode. Because V4 is not a default, React.res module will be transformed by V3.

Can you explain more? There's a lot of content in that sentence.

@mununki
Copy link
Member

mununki commented Sep 1, 2022

I can summarize this PR rescript-lang/rescript-react#49 into three things to achieve.

  1. For V4 + classic
    There are react components that are not defined with @react.component in the current React.res. It means they are working only for V3. The bsconfig.json in rescript-react has jsx configuration of V3 either. Therefore, this PR has RescriptReact.res which contains react components re-written with @react.component + @@jsxConfig({version: 4})

  2. For V4 + automatic = the new jsx transform APIs added
    https://github.com/rescript-lang/rescript-react/blob/8f31f375bc8965dad4a1c498b6ab69227f150069/src/React.res#L27:L37
    https://github.com/rescript-lang/rescript-react/blob/8f31f375bc8965dad4a1c498b6ab69227f150069/src/ReactDOM.res#L2105:L2115

  3. Type binding to Jsx in the compiler
    https://github.com/rescript-lang/rescript-react/blob/8f31f375bc8965dad4a1c498b6ab69227f150069/src/React.res#L1

So, I like to mention that 1 and 2 in this PR are outstanding stuff for JSX V4.


This is a little bit off-topic and seems proper to discuss in the PR thread.

In my opinion, we can't make a single version supporting both V3 and V4 without having React.res and RescriptReact.res both. I think it is not a happy situation for users.

How about making a breaking version?

  • v0.10.3 for JSX V3
  • v0.11.0 higher for JSX V4

@cristianoc
Copy link
Collaborator Author

Why create a new version for JSX V3?

I understand if some breaking changes might be unavoidable (or unclear how to achieve) in order to support V4.
But why a new version for V3?

@mununki
Copy link
Member

mununki commented Sep 1, 2022

Oh, I meant v0.10.3 is the current version of rescript-react. There's no need to make a new version for V3. Sorry for making a confusion.

@cristianoc
Copy link
Collaborator Author

@mattdamon108 so I think next steps are publishing a new beta version of rescript-react, finish up any testing and documentation, then invite people to test V4 in its various configurations. Correct?

@mununki
Copy link
Member

mununki commented Sep 2, 2022

@mattdamon108 so I think next steps are publishing a new beta version of rescript-react, finish up any testing and documentation, then invite people to test V4 in its various configurations. Correct?

That would be very nice.

@mununki
Copy link
Member

mununki commented Sep 2, 2022

If you agree to make the breaking version for JSX V4, then I'll fix the PR rescript-lang/rescript-react#49 accordingly.

@cristianoc
Copy link
Collaborator Author

yes

@mununki
Copy link
Member

mununki commented Sep 2, 2022

yes

Got it. 🖖 I'll fix and clean up the PR right away.

@cristianoc cristianoc changed the title JSX V4 JSX V4: investigate leftovers after release. Sep 23, 2022
@cristianoc cristianoc modified the milestones: v10.1, v11.0 Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants