-
Notifications
You must be signed in to change notification settings - Fork 38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small comment on style: this looks a little strange:
let makePropsTypeParamsSig namedTypeList =
namedTypeList
|> List.filter_map (fun (isOptional, label, _, interiorType) ->
if label = "key" || label = "ref" then None
else if isOptional then Some (extractOptionalCoreType interiorType)
else Some interiorType)
as it removes an @optional
from the type. But that annotation was added by the ppx itself.
It might indicate that the logic for doing this should probably be further back in the caller, not in this function. So that the @optional
attribute is never added in the first place. Otherwise, it could be easy to introduce bugs via refactoring without realizing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this is pre-existing, but now that the code is formatted it really jumps to the eye.
Wrapping things in let jsxMapper () =
indents everything, and groups a bunch of functions that don't really need to be grouped. How about just remove it and bring everything to the top level.
At that point, this global state:
should be created in |
Absolutely, I totally forgot this part of generating sig, this strange code was written before you introduced the |
It sounds good to me! |
Probably best to rebase on master (see #565) sooner rather than later. |
What is the core team's plan for ppx v4 to support the new jsx transform? I found some feedbacks in forum that they want it, but there is a review not to support the new jsx transform. rescript-lang/rescript-react#1 (comment)
|
One constraint is that the initial release will require no changes to rescript-react. |
The APIs for the new jsx transform are bound already here, but those are having attribute |
The PPX can issue code that silences deprecation warnings. Just like it can do for unused variables. |
Sure, the first case that comes to mind is the project which has the dependencies under React v17. But I can't claim other cases for now. Maybe we can ask it in the forum. |
Definitely something for the forum. Really need to know whom this will break. As that would make it necessary to have it configurable. |
Yes, it is workable as one of configurations in "reason": {
"react-jsx": 4,
"react-runtime": "automatic" // or "classic" like the Babel configuration
} |
I'm trying to figure out how to access the bsconfig value, but I think I'm not on right track. Can you give me a clue? |
I think that's right. bsc needs to be as fast as possible so it gets all the options passed to |
It makes sense that the separate binary seems an architectural choice for performance. |
I figured out and tested adding a new cli arg between rescript.exe and bsc.exe successfully. Now I can get a configuration value from bsconfig.json to syntax. I have a question and ask your thought about user interface and API between compiler and syntax. bsconfig.json"reason": {
"react-jsx": 3 // or 4
"react-runtime": "classic" // or "automatic"
} cli args-bs-jsx [3 or 4]
-bs-react-runtime [classic or automatic] In this regard, If we put the ppx v3 and v4 both, then we can make users migrate their projects gracefully. What do you think? |
Suggestion: This could be a good time to begin thinking about migration. Having a version flag is useful, though not enough, to support migration. The flag is what's going to be used at the end of a gradual migration. What's left is supporting intermediate migration steps when one only migrates e.g. one component, or one dependent project. |
Something to check, once the ppx version flag is in place: what happens with dependencies? I guess they will be recompiled with whatever setting is used in the current project. But one needs to verify that this is true. |
* jsx version check is conducted in compiler config side
How would you like to proceed. |
Sure, why not. I prefer keeping the open time of PR short either. Maybe, we can ship the ppx v4 as an experimental feature with V10 release, it allows us to get feedback and chances to refine the v4 and rescript-react. |
How about putting bindings for the new jsx transform directly inside the compiler? Under the |
Default: none" ); | ||
( "-jsx-runtime", | ||
Arg.String (fun txt -> jsxRuntime := txt), | ||
"Specify the jsx runtime for React, classic or automatic. Default: \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option only affects V4 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it affects V4 only.
@@ -2604,6 +2605,28 @@ and parseJsxProp p = | |||
if optional then Asttypes.Optional name else Asttypes.Labelled name | |||
in | |||
Some (label, attrExpr)) | |||
(* {...props} *) | |||
| Lbrace -> ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when this is parsed while V3 is active?
Should this be gated on version or is it OK like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be OK with V3. V9 will give us a parsing error, but V10 will parse the JSX application as below, but it will be a type error anyway. Because the definition doesn't have a spreadProps
as a prop. IMO, it doesn't make anything worse.
// lowercase
ReactDOMRe.createDOMElementVariadic(
"div",
~props=ReactDOMRe.domProps(~spreadProps=foo, ()),
[],
)
// uppercase
React.createElement(Foo.make, Foo.makeProps(~spreadProps=foo, ())),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
Just checking that nothing is broken in V3 support at the moment.
Not really reviewing V4 fully.
I think what's out there, modulo a couple of pending questions, can be merged. And work on V4, and review on it, can continue separately. |
I can put the new jsx binding into |
|
How about adding some missing binding into rescript-react, and make additional type such as |
I think this can be merged right now, then continued. |
Thanks for merging, I'm gonna make the new PR part 2. |
This PR rewrites the JSX PPX which is aligned to #521.
Here's the Spec at the time of writing this. But please check if the spec has changed as part of this PR.
A further plans
forwardRef
interfacereact/jsx-runtime
rescript-react
. PR Jsx adaptions rescript-react#1jsx.Fragment
binding into the above jsx bindingref
only forReact.forwardRef
RescriptReactErrorBoundary
impl and intf are mismatching in current version of rescript-react impl intf