-
Notifications
You must be signed in to change notification settings - Fork 347
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
Abstract bindings on ppx #768
Conversation
ppx/test/output_locations.expected
Outdated
(pos_bol 0) (pos_cnum 18))) | ||
(loc_end | ||
((pos_fname generated_locations.ml) (pos_lnum 1) | ||
(pos_bol 0) (pos_cnum 23))) | ||
(pos_bol 0) (pos_cnum 65))) |
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.
These are pretty big changes, do you know what happened?
ppx/src/reactjs_jsx_ppx.ml
Outdated
|
||
let componentLike ~loc props return = | ||
Ptyp_constr | ||
( { loc; txt = Ldot (Lident "React", "componentLike") }, |
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 and ReactDOM.domProps
are types, not bindings. But I guess it is ok :)
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.
I guess it's fine, right?
Maybe Bindings.Types.* is an overkill?
Co-authored-by: Javier Chávarri <javier.chavarri@gmail.com>
ca22de0
to
3d3728d
Compare
8395692
to
6b1b9e5
Compare
Previously to remove the legacy APIs I want to make sure we don't rely on those on the ppx. I'm doing this small abstraction to have an easy way to know which bindings we use from the ppx and improving a big the quality of this beast.
This refactor should be harmless. I have tested the location test and seems ok.