-
Notifications
You must be signed in to change notification settings - Fork 455
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
Add @as support for obj ppx #6412
Conversation
@@ -1,21 +1,38 @@ | |||
@@jsxConfig({version: 3}) | |||
|
|||
module C30 = { | |||
@obj external makeProps: (~_open: 'T_open, ~key: string=?, unit) => {"_open": 'T_open} = "" | |||
module C3A0 = { |
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 affected in the PR
Could somebody help me with the CI error? |
Had the same issue in my PR, here is the fix: ba56ffc |
I can update it when I get a chance. You're still on 2.x, right, not 3.x? @cristianoc opinions on this PR? Looks to me like it's just implementing what should already be working but wasn't. |
@@ -449,11 +475,16 @@ let process_obj (loc : Location.t) (st : external_desc) (prim_name : string) | |||
| Extern_unit -> assert false | |||
| Poly_var _ -> | |||
Location.raise_errorf ~loc | |||
"%@obj label %s does not support such arg type" name | |||
"%@obj label %s does not support such arg type" label |
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.
Separate from this PR, but these errors would be good to clean up too at some point.
@DZakh |
It should be only for external. |
Yes we are. Actually, it might be even easier for us to migrate to 3.x. But I think it would be nice to make 2.x work with V11 for other people. I'm currently on vacation, but if I have time, I'll prepare a PR. |
PStr | ||
[ | ||
{ | ||
pstr_desc = | ||
Pstr_eval | ||
( { | ||
pexp_desc = Pexp_constant (Pconst_string (alias, _)); | ||
_; | ||
}, | ||
_ ); | ||
_; | ||
}; | ||
] ) -> | ||
Some alias | ||
| _ -> None) |
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.
2ce4ea2
to
d66d436
Compare
@aspeddro Thank you. I've found an even better helper. |
It should be ready now |
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 great. One day we can hopefully get rid of this all together, but it does have some interesting use cases still.
Yep, it would be great. But for now, it's nice to have for backward compatibility |
After we removed field mangling, it happened that it was not possible to migrate to rescript@11 because rescript-relay relied on it to support queriend data having names as reserved keywords. So, as an alternative solution for the field mangling, I think we should allow creating field aliases in the obj ppx, as it works in other places.
@zth Will you be able to update rescript-relay to use
@as
instead of prefixing reserved keywords with underscore? Or should I do this? It's currently the only blocker for us to start using rescript@11 at Carla.