JSX PPX: add Jsx.element return constraint#7939
Conversation
cknitt
force-pushed
the
return-jsx-element
branch
2 times, most recently
from
64dc1a2 to
80b9809
Compare
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
| This has type: [1;31mint[0m | ||
| But it's expected to have type: [1;33mJsx.element[0m | ||
|
|
||
| In JSX, all content must be JSX elements. You can convert int to a JSX element with [1;33mReact.int[0m. No newline at end of file |
There was a problem hiding this comment.
The error messages could be further improved for this special case.
Leaving that to @zth. 🙂
There was a problem hiding this comment.
Yes, we can do this in a follow up.
| Hover src/Jsx2.resi 1:4 | ||
| {"contents": {"kind": "markdown", "value": "```rescript\nprops<string>\n```\n\n---\n\n```\n \n```\n```rescript\ntype props<'first> = {first: 'first}\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Jsx2.resi%22%2C0%2C0%5D)\n"}} | ||
| {"contents": {"kind": "markdown", "value": "```rescript\nJsx.element\n```\n\n---\n\n```\n \n```\n```rescript\ntype Jsx.element\n```\nGo to: [Type definition](command:rescript-vscode.go_to_location?%5B%22Jsx.res%22%2C25%2C0%5D)\n"}} |
There was a problem hiding this comment.
Same here, is this expected? If so, why?
There was a problem hiding this comment.
@zth I updated this PR, and this change here is now the only remaining open issue.
However, I think it needs to be solved in analysis. As far as I can see, the hover is on make, so the output was already incorrect before (showing the props type instead of the full function type) and is now incorrect in a different way. 🙂
| module M4: { | ||
| @react.component | ||
| let make: (~first: string, ~fun: string=?, ~second: string=?) => React.element | ||
| let make: (~first: string, ~fun: string=?, ~second: string=?) => Jsx.element |
There was a problem hiding this comment.
Interesting change. I would assume we'd still want it to be React.element?
There was a problem hiding this comment.
As the return type of all Jsx component functions is now constrained to Jsx.element, it makes sense that that's also what turns up when generating the interface.
cknitt
force-pushed
the
return-jsx-element
branch
3 times, most recently
from
bbba8c4 to
1e83838
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
cknitt
force-pushed
the
return-jsx-element
branch
from
1e83838 to
823a6de
Compare
| let make = (type a, ~foo) => { | ||
| module T = unpack(foo: T with type t = a) | ||
| foo | ||
| React.null |
There was a problem hiding this comment.
I guess there's no way to still return foo here, just wrapped in the relevant jsx element fn...?
There was a problem hiding this comment.
Only with Obj.magic I am afraid, if we wanted the JS output to stay the same.
zth
left a comment
zth
left a comment
There was a problem hiding this comment.
Just 1 small comment, but looks good!
2 participants
Fixes #7722