-
Notifications
You must be signed in to change notification settings - Fork 470
Support async component for React Server component in JSX v4 #6399
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
Changes from 7 commits
2fc6d8f
38239bc
2c90fb4
efed71f
8fbaee4
a0402ae
e896491
8126a13
4d1e792
9d283ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,3 +63,28 @@ let removeArity binding = | |
| _ -> expr | ||
in | ||
{binding with pvb_expr = removeArityRecord binding.pvb_expr} | ||
|
||
let add_async_attribute ~async (body : Parsetree.expression) = | ||
if async then | ||
{ | ||
body with | ||
pexp_attributes = | ||
({txt = "res.async"; loc = Location.none}, PStr []) | ||
:: body.pexp_attributes; | ||
} | ||
else body | ||
|
||
let is_async : Parsetree.attribute -> bool = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would you move this function too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does it look? 9d283ba There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks great. |
||
fun ({txt}, _) -> txt = "async" || txt = "res.async" | ||
|
||
let async_component ~async expr = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should not move as it's specific to the ppx |
||
if async then | ||
let open Ast_helper in | ||
Exp.apply | ||
(Exp.ident | ||
{ | ||
loc = Location.none; | ||
txt = Ldot (Lident "JsxPPXReactSupport", "asyncComponent"); | ||
}) | ||
[(Nolabel, expr)] | ||
else expr |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
let f = a => Js.Promise.resolve(a + a) | ||
|
||
module C0 = { | ||
@react.component | ||
let make = async (~a) => { | ||
let a = await f(a) | ||
<div> {React.int(a)} </div> | ||
} | ||
} | ||
|
||
module C1 = { | ||
@react.component | ||
let make = async (~status) => { | ||
switch status { | ||
| #on => React.string("on") | ||
| #off => React.string("off") | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
let f = a => Js.Promise.resolve(a + a) | ||
|
||
module C0 = { | ||
type props<'a> = {a: 'a} | ||
|
||
let make = async ({a, _}: props<_>) => { | ||
let a = await f(a) | ||
ReactDOM.jsx("div", {children: ?ReactDOM.someElement({React.int(a)})}) | ||
} | ||
let make = { | ||
let \"AsyncAwait$C0" = (props: props<_>) => JsxPPXReactSupport.asyncComponent(make(props)) | ||
|
||
\"AsyncAwait$C0" | ||
} | ||
} | ||
|
||
module C1 = { | ||
type props<'status> = {status: 'status} | ||
|
||
let make = async ({status, _}: props<_>) => { | ||
switch status { | ||
| #on => React.string("on") | ||
| #off => React.string("off") | ||
} | ||
} | ||
let make = { | ||
let \"AsyncAwait$C1" = (props: props<_>) => JsxPPXReactSupport.asyncComponent(make(props)) | ||
|
||
\"AsyncAwait$C1" | ||
} | ||
} |
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.
Aren't these functions already in the file that deals with async transformations?
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.
If you mean the files that deal with async transformation are
ast_async.ml
orast_attributes.ml
, thesyntax
module doesn't have thefrontend
as dependency yet. That's why I added here again.Do you want me to add the frontend module to deps?
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.
Oops, not able to do that due do circular deps: frontend -> common -> syntax
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.
Looks like front-end already accesses
ml
:(library (name frontend) (wrapped false) (flags (:standard -w +a-4-9-40-42-70)) (libraries common ml))
and
ml
is where e.g.ast_untagged_variants.ml
is.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.
Maybe I'm missing something.
syntax
can accessml
, but notfrontend
that has async transformation functions.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.
Perhaps my wording is confusing. Actually, this is correct: frontend <- common <- syntax
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.
Put it where
ast_untagged_variants.ml
is, and change nothing else.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's not allowed to use anything that is not already in
ml
, obviously.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.
Got it. 8126a13
I was going to put only
ast_async.ml
inml
, but I thought it would be nice to haveast_await.ml
in the same place, so I moved it. 4d1e792