-
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
JSX v4 and @string / @int #5724
Comments
Not clear why this is eaten up, the type of fields should be preserved. |
AFAIK this It only works as part of an |
This is how the result of processing looks like: module EE = {
type props<
'animated,
'barStyle,
'hidden,
'backgroundColor,
'translucent,
'networkActivityIndicatorVisible,
'showHideTransition,
> = {
animated?: 'animated,
barStyle?: 'barStyle,
hidden?: 'hidden,
backgroundColor?: 'backgroundColor,
translucent?: 'translucent,
networkActivityIndicatorVisible?: 'networkActivityIndicatorVisible,
showHideTransition?: 'showHideTransition,
}
@module("react-native")
external make: React.componentLike<
props<
bool,
@string
[
| #default
| @as("light-content") #lightContent
| @as("dark-content") #darkContent
],
bool,
string,
bool,
bool,
showHideTransition,
>,
React.element,
> = "StatusBar"
} The type in question ends up as parameter to the type definition. So it looks like a combination of:
So that as soon as something changes this brittle usage breaks. |
Yes, I think this is why. |
@cknitt @mattdamon108 considering how brittle 1 Make a suggestion for alternative bindings that do not use this 2 Turn this use into a warning, or perhaps even an error Thoughts? In fact, longer term, review |
If this is not possible to solve in the PPX (and I think it isn't?), then yes, only the two options described by @cristianoc remain:
|
A more elaborate (implementation-wise) version of 1 would be to auto-transform instead of (or in addition to) giving an error/warning. Bonus points: make this part of reformat so old style code gets reformatted to new style. Slightly related: it would be nice to have a de-sugaring mode for debugging where all the transformations are performed at reformatting time, so one sees exactly what code gets generated (after JSX and other annotations are processed) without even leaving the editor. |
I didn't look into the code related to |
There are several aspects about syntax, semantics, and backwards compatibility.
What this means, is that it does not seem possible to salvage a mechanism such as this one for So there seems to be a bunch of exploration to be done beyond this specific issue. |
I have not investigated all the uses of let aa = #"this-or-that"
let bb = #45 One needs to check carefully, but it is conceivable that they can just be deprecated and removed over time. |
I guess for this issue, one can just start with a small step and remove them from component definitions. |
Ok, so then the only remaining cause of action is to produce an error when these attributes are used with JSX v4. This will avoid runtime errors and force library authors to update their libraries accordingly.
Right, there is no real need now anymore for this usage of
If you are talking about rescript-react-native in particular, I have already done this, see rescript-react-native/rescript-react-native#770. |
Frankly, I didn't know this usage yet EDIT: not remove it deprecate it first 😉 |
I think the following would be ideal:
|
BTW, regarding deprecating |
@cristianoc What about
? Don't we want to do that? Or do we do it later (in 11.0)? |
Forgot about this, but I guess we can deprecate all uses in 11.0, whether or not in a component. |
Consider the following binding (part of rescript-react-native):
Now
<StatusBar barStyle=#darkContent />
gives the following output with JSX v3:but with JSX v4 we get
causing a runtime exception.
The problem is of course not limited to React Native, but will affect any React component using
@string
(or, similarly,@int
) for any of its props./cc @mattdamon108 @cristianoc
The text was updated successfully, but these errors were encountered: