rescript-react icon indicating copy to clipboard operation
rescript-react copied to clipboard

Update new jsx transform apis, bind to Jsx in compiler

Open mununki opened this issue 2 years ago • 5 comments

This PR introduces the new Jsx module in the compiler. This is an initial proposal stage that has objectives and constraints as below.

  • Adding The new jsx transform APIs which the JSX PPX V4 can call
  • No opaque type to keep the backward compatibility with the existing React modules
    type element = Jsx.element
    
  • Peer dependency of React@17 or higher
  • ~~The new domProps and props types which are declared with @optional attribute~~ Added in the compiler https://github.com/rescript-lang/rescript-compiler/pull/5484
  • ~~The new JsxDOMStyle module~~ Added in the compiler https://github.com/rescript-lang/rescript-compiler/pull/5484

I don't have a clear picture of Jsx in the future. It could define the primitive types, functions, and modules that are vendors for JSX family, such as React, SolidJs, Remix in future. But I focus to keep the backward compatibility and support for the new jsx mode of React runtime in this stage for now.

mununki avatar Jun 30 '22 17:06 mununki

This looks good so far. Let's keep it open until the whole V4 ppx is finalised.

cristianoc avatar Jun 30 '22 18:06 cristianoc

Side comment: I verified that V4 & Jsx don't break my test project having dependencies of V3 modules. So, I tried to run the battle-test of V10 & V4 & Jsx on my company projects. I encountered some errors before verifying that V4 & classic, V4 & Jsx will not break the backward compatibility. I'll leave the issue in the compiler.

mununki avatar Jul 01 '22 00:07 mununki

Side comment: I verified that V4 & Jsx don't break my test project having dependencies of V3 modules. So, I tried to run the battle-test of V10 & V4 & Jsx on my company projects. I encountered some errors before verifying that V4 & classic, V4 & Jsx will not break the backward compatibility. I'll leave the issue in the compiler.

Can you expand a bit on this? Did you find some problem, and will document that in a new issue? Or were no problems found?

cristianoc avatar Jul 01 '22 01:07 cristianoc

Side comment: I verified that V4 & Jsx don't break my test project having dependencies of V3 modules. So, I tried to run the battle-test of V10 & V4 & Jsx on my company projects. I encountered some errors before verifying that V4 & classic, V4 & Jsx will not break the backward compatibility. I'll leave the issue in the compiler.

Can you expand a bit on this? Did you find some problem, and will document that in a new issue? Or were no problems found?

I ran V10 & V4 & Jsx on my test project which is a small RescriptReact app. It has dependencies of the RescriptReact module I made with V3 bsconfig. I've tested the scenarios (V3, V4 & classic, V4 & Jsx), and they worked fine.

But I ran V10 compiler in V3 mode on large applications(company projects), it throws several errors. I guess it is not related to V4 or Jsx. So I left the issue in the compiler https://github.com/rescript-lang/rescript-compiler/issues/5493

Let me know if anything I can help on that.

mununki avatar Jul 01 '22 01:07 mununki

I totally agree on the comment from @ryyppy https://github.com/rescript-lang/rescript-compiler/pull/5484#discussion_r910144004 Here is my thought for the future plan. What do you think?

  1. Remove Jsx from rescript-react. If the namespace Jsx is occupied here in rescript-react, there will be a problem(in future graceful migrations) when we add Jsx in the compiler. Jsx should be containing the primitive types, functions, modules which are vendors for the 3rd party modules, such as rescript-react, or rescript-solidjs, etc.

  2. Add the new jsx APIs in React.res and ReactDOM.res If the upgrade plan is that V4 & the new jsx transform will be shipped with upgrading rescript-react, then we don't need to add a new module here. It is enough to add the new jsx transform React APIs into React.res and ReactDOM.res Those APIs are just for the React anyway.

  3. Add Jsx in the compiler and bind the types to rescript-react The initial version of Jsx can be added to the compiler for the experimental feature. Then we can bind the rescript-react to Jsx. For example,

// React.res
type element = Jsx.element

// ReactDOM.res
@module("react/jsx-runtime")
external jsxKeyed: (string, JsxDOM.domProps, string) => Jsx.element = "jsx" // Jsx in the compiler

Does it make sense to you?

mununki avatar Jul 02 '22 07:07 mununki

The breaking change is introduced by merging RescriptReact.res into React.res.

Side comment: I realized why some of react component binding was not written with @react.component. Simply it is not possible, because the ppx will transform @react.componet using React.res itself. 🥲 Therefore, I write the binding without @react.component, but it is compatible with ppx V4.

mununki avatar Sep 02 '22 13:09 mununki

How does one test this? Perhaps one way is to create a separate sample project, which installs the compiler from master, and the rescript-react from this PR, and tests the various things that have changed?

cristianoc avatar Sep 05 '22 03:09 cristianoc

How does one test this? Perhaps one way is to create a separate sample project, which installs the compiler from master, and the rescript-react from this PR, and tests the various things that have changed?

Yes exactly. That is how I develop and test in my local env.

mununki avatar Sep 05 '22 07:09 mununki

How does one test this? Perhaps one way is to create a separate sample project, which installs the compiler from master, and the rescript-react from this PR, and tests the various things that have changed?

Yes exactly. That is how I develop and test in my local env.

Great! Would you create such a project? That would also be the base example to ask people for feedback. As it will take a while before both are published in a way that people can try.

cristianoc avatar Sep 05 '22 08:09 cristianoc

How does one test this? Perhaps one way is to create a separate sample project, which installs the compiler from master, and the rescript-react from this PR, and tests the various things that have changed?

Yes exactly. That is how I develop and test in my local env.

Great! Would you create such a project? That would also be the base example to ask people for feedback. As it will take a while before both are published in a way that people can try.

Sure! I'll work on it.

mununki avatar Sep 05 '22 08:09 mununki

Here is the example project for test https://github.com/mattdamon108/react-jsx-ppx-v4

  • ReScript compiler@master
  • rescript-react@jsx (this PR branch)

mununki avatar Sep 05 '22 14:09 mununki

Here is the example project for test https://github.com/mattdamon108/react-jsx-ppx-v4

  • ReScript compiler@master
  • rescript-react@jsx (this PR branch)

Great. npm install failed for me but yarn worked (after waiting a while).

% npm install
npm WARN deprecated [email protected]: See https://github.com/lydell/source-map-resolve#deprecated
npm WARN deprecated [email protected]: This SVGO version is no longer supported. Upgrade to v2.x.x.
npm ERR! code 1
npm ERR! git dep preparation failed
npm ERR! command /opt/homebrew/Cellar/node/18.3.0/bin/node /opt/homebrew/lib/node_modules/npm/bin/npm-cli.js install --force --cache=/Users/cristianocalcagno/.npm --prefer-offline=false --prefer-online=false --offline=false --no-progress --no-save --no-audit --include=dev --include=peer --include=optional --no-package-lock-only --no-dry-run
npm ERR! npm WARN using --force Recommended protections disabled.
npm ERR! npm ERR! code ETARGET
npm ERR! npm ERR! notarget No matching version found for rescript@^10.1.0.
npm ERR! npm ERR! notarget In most cases you or one of your dependencies are requesting
npm ERR! npm ERR! notarget a package version that doesn't exist.
npm ERR! 
npm ERR! npm ERR! A complete log of this run can be found in:
npm ERR! npm ERR!     /Users/cristianocalcagno/.npm/_logs/2022-09-05T15_55_52_418Z-debug-0.log

cristianoc avatar Sep 05 '22 15:09 cristianoc

npm install failed for me but yarn worked (after waiting a while).

Oh, didn't notice. I'll check the installation with npm.

mununki avatar Sep 05 '22 16:09 mununki

Fixed the installation error with npm https://github.com/mattdamon108/react-jsx-ppx-v4/commit/01720fe5dd3b82ac69c7e9ae90d87c00fba704d1 after fixing the rescript version https://github.com/rescript-lang/rescript-react/pull/49/commits/9ac34b0ee02a0e3598826229741fb01a3812ad7c The reason was rescript-react#jsx PR has an unavailable dependency.

mununki avatar Sep 05 '22 16:09 mununki

Time for a post in the forum asking people to test PPX v4?

cristianoc avatar Sep 05 '22 17:09 cristianoc

The breaking change is introduced by merging RescriptReact.res into React.res.

Side comment: I realized why some of react component binding was not written with @react.component. Simply it is not possible, because the ppx will transform @react.componet using React.res itself. 🥲 Therefore, I write the binding without @react.component, but it is compatible with ppx V4.

@mattdamon108 now I'm wondering: should rescript-react move inside the compiler? It seems to be tightly coupled with the JSX, and tight coupling at a distance is a recipe for problems.

Inside the compiler, it could serve both V3 and V4 depending on the mode, as the mode can switch half way through a single file.

cristianoc avatar Sep 13 '22 09:09 cristianoc

@mattdamon108 now I'm wondering: should rescript-react move inside the compiler? It seems to be tightly coupled with the JSX, and tight coupling at a distance is a recipe for problems.

Inside the compiler, it could serve both V3 and V4 depending on the mode, as the mode can switch half way through a single file.

Before this PR https://github.com/rescript-lang/rescript-compiler/pull/5661, I thought that moving the rescript-react to the compiler would solve the part of problems. Now it looks solving most problems. But one thing left to think is that the compiler has the bindings for React. Doesn’t it seems too tightly coupled?

mununki avatar Sep 13 '22 12:09 mununki

The alternative is to provide a compatibility module ReactV3. So one can take an existing v3 app and build it with the new rescript-react and version:3 as long as open ReactV3 is used.

cristianoc avatar Sep 13 '22 13:09 cristianoc

The alternative is to provide a compatibility module ReactV3. So one can take an existing v3 app and build it with the new rescript-react and version:3 as long as open ReactV3 is used.

I think this is better choice. Even if rescript-react is moved in the compiler, it has to have a version name anyway. I’ll revive the old React module as ReactV3.

mununki avatar Sep 13 '22 13:09 mununki

Regarding PR to remove the key from the props type https://github.com/rescript-lang/syntax/pull/635, I've added the addKeyProp into React.res and tested in my example project, it seems fine.

// original
<Foo key="k" x="x" />

// converted
React.createElement(Foo.make, React.addKeyProp(({x: "x"}: Foo.props<_>), "k")),

mununki avatar Sep 16 '22 09:09 mununki

Regarding PR to remove the key from the props type rescript-lang/syntax#635, I've added the addKeyProp into React.res and tested in my example project, it seems fine.

// original
<Foo key="k" x="x" />

// converted
React.createElement(Foo.make, React.addKeyProp(({x: "x"}: Foo.props<_>), "k")),

How about adding instead Jsx.addKeyProp so it stays with the compiler? So one can mostly test V4 without updating rescript-react, as long as only basic features are used.

cristianoc avatar Sep 16 '22 10:09 cristianoc

How about adding instead Jsx.addKeyProp so it stays with the compiler? So one can mostly test V4 without updating rescript-react, as long as only basic features are used.

Very good idea. I’ll add it instead.

mununki avatar Sep 16 '22 12:09 mununki

Jsx.addKeyProp https://github.com/rescript-lang/rescript-compiler/pull/5664

mununki avatar Sep 16 '22 17:09 mununki

Temporarily made CI use the latest commit on master -- this will go when a new alpha version of the compiler is released.

I guess this only needs a compatibility module for V3 now. E.g.

module C = {
  @@jsxConfig{version:3}
  open React.V3

  @react.component
  let make = ...
}

or at the project level, take an existing project and:

  • install future compiler 10.1
  • install future rescript-react 11
  • add open React.V3 to the global setting

cristianoc avatar Sep 19 '22 02:09 cristianoc

@mattdamon108 how does that ^ backwards compatibility experience sound like?

cristianoc avatar Sep 19 '22 02:09 cristianoc

@mattdamon108 how does that ^ backwards compatibility experience sound like?

Sounds very promising. Sorry for being late to make React.V3 module. It is a simple task. I'll make it done soon. (let me have 1 hour top after grabbing a lunch now 😃 )

mununki avatar Sep 19 '22 02:09 mununki

It is more tricky than I expected. 😄 https://github.com/rescript-lang/rescript-react/pull/49/commits/86401bcb5bb5b60d13c685a787af78bdbe996e4b

I couldn't put it inside React.res due to circular referencing.

mununki avatar Sep 19 '22 06:09 mununki

It is more tricky than I expected. 😄 86401bc

I couldn't put it inside React.res due to circular referencing.

Would you be able to try a big project, see if everything still builds (and generated identical JS) with V3 compatibility?

I think the bsconfig option is: "bsc-flags": ["-open ReactV3"],

cristianoc avatar Sep 19 '22 08:09 cristianoc

Would you be able to try a big project, see if everything still builds (and generated identical JS) with V3 compatibility?

I think the bsconfig option is: "bsc-flags": ["-open ReactV3"],

I think I can run more thoroughly with this fix of rescript-relay https://github.com/zth/rescript-relay/issues/391 Most of components not using relay seems fine though. I'll let you know the result tomorrow.

mununki avatar Sep 19 '22 16:09 mununki

Would you be able to try a big project, see if everything still builds (and generated identical JS) with V3 compatibility?

I've ran on my company project, face this error.

image
  • compiler#master
  • rescript-react#jsx
  • "bs-flags": ["-open ReactV3"]

The error indicates that dependency modules are using React.res in rescript-react. Doesn't bs-flag affect the dependency?

mununki avatar Sep 21 '22 09:09 mununki