Skip to content
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

Closed
cknitt opened this issue Oct 8, 2022 · 18 comments · Fixed by rescript-lang/syntax#672
Closed

JSX v4 and @string / @int #5724

cknitt opened this issue Oct 8, 2022 · 18 comments · Fixed by rescript-lang/syntax#672
Milestone

Comments

@cknitt
Copy link
Member

cknitt commented Oct 8, 2022

Consider the following binding (part of rescript-react-native):

@react.component @module("react-native")
external make: (
  ~animated: bool=?,
  ~barStyle: @string
  [
    | #default
    | @as("light-content") #lightContent
    | @as("dark-content") #darkContent
  ]=?,
  ~hidden: bool=?,
  ~backgroundColor: string=?,
  ~translucent: bool=?,
  ~networkActivityIndicatorVisible: bool=?,
  ~showHideTransition: showHideTransition=?,
) => React.element = "StatusBar"

Now <StatusBar barStyle=#darkContent /> gives the following output with JSX v3:

React.createElement(ReactNative.StatusBar, {
  barStyle: "dark-content"
});

but with JSX v4 we get

JsxRuntime.jsx(ReactNative.StatusBar, {
  barStyle: "darkContent"
});

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

@cknitt cknitt added this to the v10.1 milestone Oct 8, 2022
@cristianoc
Copy link
Collaborator

cristianoc commented Oct 8, 2022

Not clear why this is eaten up, the type of fields should be preserved.
Is there perhaps a smaller repro without JSX, e.g. whenever @string is inside a type definition or something?

@cknitt
Copy link
Member Author

cknitt commented Oct 8, 2022

AFAIK this @string/@as usage does not work as part of a normal type definition.

It only works as part of an external definition, and JSX v3 is using @obj external makeProps ....

@cristianoc
Copy link
Collaborator

cristianoc commented Oct 8, 2022

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:

  • weak semantics of @string annotation
  • relying on some details of the transformation

So that as soon as something changes this brittle usage breaks.

@mununki
Copy link
Member

mununki commented Oct 9, 2022

It only works as part of an external definition, and JSX v3 is using @obj external makeProps ....

Yes, I think this is why.

@cristianoc
Copy link
Collaborator

cristianoc commented Oct 9, 2022

@cknitt @mattdamon108 considering how brittle @string is, one possibility would be to:

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 @int / @string in general see if it could be replaced completely or made more robust. For example, there's no reason one would not want the same kind of representation to be used when exporting to JS. The one-sided focus on importing does not seem to be justified.

@cknitt
Copy link
Member Author

cknitt commented Oct 9, 2022

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:

  1. Make this a warning or an error (I am leaning towards the latter). Include a suggestion for an alternative in the error message (just use [ | #default | #"light-content" | #"dark-content" ] in my above example). The problem with this approach is that, in order to upgrade to JSX v4, people will need to make sure that all of their project's dependencies have been updated accordingly.

  2. Generalize @int / @string for polymorphic variants so that they work outside of external definitions, too. IMHO this would be the preferable solution, but I do not know how hard it would be to implement and if it would be doable in the scope of release 10.1.

@cristianoc
Copy link
Collaborator

cristianoc commented Oct 9, 2022

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.

@mununki
Copy link
Member

mununki commented Oct 9, 2022

I didn't look into the code related to @string and @int yet. Do we really need @string and @intinside even outside external? If it is possible to emit the expected js output with @as only, I think @string and @int don't need.
I always think the syntax needs to get more easier and easier for the newcomer and beginner. The fewer attributes make it more friendly to JS users.

@cristianoc
Copy link
Collaborator

cristianoc commented Oct 9, 2022

There are several aspects about syntax, semantics, and backwards compatibility.

  • Conceptually, it seems like different aspects are mixed. One is the runtime representation of things, and this is one such example. Another aspect is the code generated, and things like @module are an example. The third one is FFI. Often these are mixed, as in this example, but they don't have to, in principle.

  • Attributes are not sticky: they are invisible things added on top, so it's easy to "lose" them. For example, if you add attributes to a type definition of a polymorphic variant type, such as the type for barStyle, they won't be propagated as every use of a polymorphic variant has its type inferred automatically. Contrast this with nominal type declarations such as records, where one can store arbitrary amount of information via attributes in a solid way.
    In this case, attributes are picked up when attached to an external, as the external comes with a type declaration and no inference is performed. This gives a mechanism, but the mechanism is brittle, as it does not survive type aliasing.

What this means, is that it does not seem possible to salvage a mechanism such as this one for @string, associated with polymorphic variants, without making it fragile. So I think it should just be removed, as much as it is possible. Sometimes in future.

So there seems to be a bunch of exploration to be done beyond this specific issue.

@cristianoc
Copy link
Collaborator

I have not investigated all the uses of @string and @int for poly variants, but for the simples cases, both things are possible today:

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.

@cristianoc
Copy link
Collaborator

I guess for this issue, one can just start with a small step and remove them from component definitions.

@cknitt
Copy link
Member Author

cknitt commented Oct 9, 2022

What this means, is that it does not seem possible to salvage a mechanism such as this one for @string, associated with polymorphic variants, without making it fragile.

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.

One needs to check carefully, but it is conceivable that they can just be deprecated and removed over time.

Right, there is no real need now anymore for this usage of @as (whether with @obj or with @react.component external definitions), except that maybe #lightContent is easier to type than #"light-content" (but I think for bindings it is preferable anyway to stay as close as possible to the JS original).

I guess for this issue, one can just start with a small step and remove them from component definitions.

If you are talking about rescript-react-native in particular, I have already done this, see rescript-react-native/rescript-react-native#770.

@mununki
Copy link
Member

mununki commented Oct 9, 2022

I have not investigated all the uses of @string and @int for poly variants, but for the simples cases, both things are possible today:

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.

Frankly, I didn't know this usage yet #45. I think no need @string and @int, even @as for polymorphic variants. I'll look into it and remove it from external usage for component binding.

EDIT: not remove it deprecate it first 😉

@cknitt
Copy link
Member Author

cknitt commented Oct 10, 2022

I think the following would be ideal:

  1. For JSX 3, deprecate this usage.
  2. For JSX 4, produce an error for this usage.

@cknitt
Copy link
Member Author

cknitt commented Oct 10, 2022

BTW, regarding deprecating @string in externals altogether later on, there is also the usage for event listeners which will still be required: https://rescript-lang.org/docs/manual/latest/bind-to-js-function#special-case-event-listeners

@cknitt
Copy link
Member Author

cknitt commented Oct 12, 2022

@cristianoc What about

For JSX 3, deprecate this usage.

? Don't we want to do that? Or do we do it later (in 11.0)?

@cristianoc
Copy link
Collaborator

Forgot about this, but I guess we can deprecate all uses in 11.0, whether or not in a component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants