Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

JSX 4: component name when created without @react.component #695

Closed
cknitt opened this issue Oct 23, 2022 · 29 comments
Closed

JSX 4: component name when created without @react.component #695

cknitt opened this issue Oct 23, 2022 · 29 comments
Labels

Comments

@cknitt
Copy link
Member

cknitt commented Oct 23, 2022

When using JSX 4, it is possible to create a component without @react.component by just creating a module with a props type and a function make: props => React.element. This can be useful in some cases, or even required due to nominal typing if we want to ensure that multiple components share the same props type.

However, if we create a component that way, its name will just be make. For example, when rendering the following component

module A = {
  type props = {}
  let make = (_: props) => React.string("A")
}

module B = {
  type props = {}
  let make = (_: props) => React.string("B")
}

type props = {}

let make = (_: props) =>
  <div>
    <A />
    <B />
  </div>

it looks like this in the React Dev Tools components inspector:

Bildschirmfoto 2022-10-23 um 13 09 03

@mununki
Copy link
Member

mununki commented Oct 23, 2022

Good catch!
One of the main jobs of @react.component is making the react component function name starting with capitalized letter.

@cknitt
Copy link
Member Author

cknitt commented Oct 23, 2022

Right, but how can we support the above use case where the user wants to specify the props type himself? Could we also just add @react.component to the make function, and the PPX would detect this as a different case (as there are no labeled arguments) and make sure that the component function name is set correctly, but do no additional magic?

@cristianoc
Copy link
Contributor

This is still a bit up in the air. You can use the same thing that the ppx does now, and define a capitalized quoted id.

The current situation is that there isn't a nice language level mechanism to cover the complete picture, including the default export normally used by components, and capitalized name.

@cristianoc
Copy link
Contributor

The default is the more delicate aspect, so perhaps worth looking at it in isolation. Making this up as we speak.

So at the language level one wants to have the notion of module with a default value, and this should map nicely to JS's notion of default export.
So at the language level, it could just be that a module has a value called default.
This would mean different things whether the module is an inner module or a file. In the case of inner module, it would be literally just a value called default. Represented in a way that does not make JS unhappy so some mangling of the name as currently done by the compiler.
In the case of a file, it would be whatever the current module (from bsconfig) notion of default export, and access mean. So what's generated could be accessed from JS, without any change.
In terms of JSX, the name make would be dropped in favour of default.

Some implicit convention, or explicit mechanism, could be used to give the name of the module to the default value.

@cristianoc
Copy link
Contributor

Treating inner modules and files differently is a bit of a complication, when e.g. functors are used. One can explore several things.
One possibility is to also take on another issue: the fact that Foo.bar might or might not access a local module, depending on what's in scope.
If one were to separate local module access, from file access, then that would be a clean separation point where to reconcile things like treatment of default, as well as making dependencies more explicit and not dependent on what's in scope (opened modules).

Needless to say, this is very speculative and not for the short term for sure.

@cristianoc
Copy link
Contributor

Combining with the exploration of dynamic import

// Counter.res has @react.component let default = ...

...
   module C = await import(Counter)
   <C .../>
...

@cknitt
Copy link
Member Author

cknitt commented Oct 23, 2022

IMHO we should support the use case of the user specifying a (possibly shared) props type himself out of the box with the upcoming JSX 4 release.

Yes, the user could do something like this manually to get the right component function name, but this is quite cumbersome:

type sharedProps = {x: string}

module A = {
  type props = sharedProps

  let make = {
    let \"A" = ({x}) => React.string("A: " ++ x)
    \"A"
  }
}

Why can't we extend the PPX so that we can have

type sharedProps = {x: string}

module A = {
  type props = sharedProps

  @react.component
  let make = ({x}) => React.string("A: " ++ x)
}

and the PPX does just the function name thing in this case?

@cristianoc
Copy link
Contributor

IMHO we should support the use case of the user specifying a (possibly shared) props type himself out of the box with the upcoming JSX 4 release.

Yes, the user could do something like this manually to get the right component function name, but this is quite cumbersome:

type sharedProps = {x: string}

module A = {
  type props = sharedProps

  let make = {
    let \"A" = ({x}) => React.string("A: " ++ x)
    \"A"
  }
}

Why can't we extend the PPX so that we can have

type sharedProps = {x: string}

module A = {
  type props = sharedProps

  @react.component
  let make = ({x}) => React.string("A: " ++ x)
}

and the PPX does just the function name thing in this case?

This is doable, probably requiring some additional annotation (e.g. an argument to @react.component).

@cristianoc
Copy link
Contributor

Though I would not change the syntax, still (~x) => ...

@cknitt
Copy link
Member Author

cknitt commented Oct 23, 2022

What I meant was that I can already write

module A = {
  type props = sharedProps
  let make = ({x}) => React.string("A: " ++ x)
}

now and it will work without any additional attribute, except for the incorrect function name. So I wanted to add an attribute (whether the existing @react.component or a new one) that just sets the function name to the module name. In this case there would be no labeled arguments.

You are proposing something like this instead, right?

module A = {
  @react.component(sharedProps)
  let make = (~x) => React.string("A: " ++ x)
}

where the @react.component would work as usual with the labeled args, but use the provided props type?

@mununki
Copy link
Member

mununki commented Oct 23, 2022

Using argument for @react.component to make quoted id for make function of react component is bit easy job. Or let’s say @react.componentName seperately.
But I want to explore what @cristianoc says about module. It can resolve the dynamic import for module either.

@mununki
Copy link
Member

mununki commented Oct 23, 2022

// @react.component
module A = {
  @react.component
  let make = (~x, ~y) => React.string(x ++ y)
}

// @react.componentName
module B = {
  type props = {x: string, y: string}
  @react.componentName
  let make = props => React.string(props.x ++ props.y)
}

// No attributes
module C = {
  type props = {x: string, y: string}
  let make = props => React.string(props.x ++ props.y)
  let make = {
    let \"C" = (props: props) => make(props)
    \"C"
  }
}

@cristianoc
Copy link
Contributor

This variation might actually work.

The ReScript

// Foo.res
let default = 3

Could compile to this JS:

// Foo.js
let Foo = 3
export default Foo

So default would be represented at runtime with the name of the enclosing module. The same rule can apply to files and inner modules, so there's no inconsistency.

This should respect the rule of a single capitalised export where needed.

@cknitt
Copy link
Member Author

cknitt commented Oct 23, 2022

Nice! But then we would need to change JSX to use default instead of make?

@cristianoc
Copy link
Contributor

Yes a rename would be necessary. Can be done incrementally helped by config settings, in case this works out without issues.

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2022

You are proposing something like this instead, right?

module A = {
  @react.component(sharedProps)
  let make = (~x) => React.string("A: " ++ x)
}

where the @react.component would work as usual with the labeled args, but use the provided props type?

Actually I think this would be quite nice, as it would allow people to specify a shared props type where needed without having to change anything else in their code.

One example where a shared prop type is needed is when you have an API that takes a React.component<someProps> as a param/prop and you want to implement a component that can be passed to that API.

@mununki
Copy link
Member

mununki commented Oct 24, 2022

I have a question regarding this example:

module A = {
  @react.component(sharedProps)
  let make = (~x) => React.string("A: " ++ x)
}

What does type sharedProps look like?

type sharedProps = {
  x: string
}

Is it?

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2022

Yes

@mununki
Copy link
Member

mununki commented Oct 24, 2022

Yes

Got it. Can you help me to understand what demanded usages are? If type sharedProps looks like it, then just making a component with @react.component seems okay except all the props types are nominally different. Is this to avoid it?

@cknitt
Copy link
Member Author

cknitt commented Oct 24, 2022

Yes, exactly. Use case is as described here:

One example where a shared prop type is needed is when you have an API that takes a React.component as a param/prop and you want to implement a component that can be passed to that API.

E.g., React Navigation. Let's say (very simplified) you have bindings giving you a Screen module like this

type screenProps = { navigation: navigation, route: route }

module Screen: {
  @react.component
  let make: (
    ~name: string,
    ~component: React.component<screenProps>,
    ...
  ) => React.element
}

and you have a component SomeComponent:

@react.component
let make = (~navigation: navigation, ~route: route) => ...

and want to define a screen as follows:

<Screen
  name="SomeScreen"
  component={SomeComponent.make}
  ...
/>

Then this won't work because the props types don't match.

@mununki
Copy link
Member

mununki commented Oct 24, 2022

Ah! I remember this post https://forum.rescript-lang.org/t/call-for-help-2-test-jsx-v4/3781/9?u=moondaddi
I think your suggestion seems like a much easier and more intuitive solution. I always think that the module function(functor) is not a good suggestion for the Js developer.

@mununki
Copy link
Member

mununki commented Oct 24, 2022

type sharedProps<'a> =// original
@react.component(sharedProps)
let make = (~x) => ...

// converted to
type props<'a> = sharedProps<'a> // tricky part here, when shareProps has type parameter
let make = ({x}: props<_>) => ...
let make = {
  let \"Foo" = props => make(props)
  \"Foo"
}

I think there is a tricky part there.

@mununki
Copy link
Member

mununki commented Oct 25, 2022

I'll grab some time to implement this feature soon. I expect that a few code changes will make this work.

@cknitt
Copy link
Member Author

cknitt commented Oct 25, 2022

That's great! Thanks a lot! (If it turns out to be too complicated, the @react.componentName idea is a good alternative.)

@mununki
Copy link
Member

mununki commented Oct 29, 2022

I'd like to take another issue here regarding the function component name starting with lowercase which would lead to breaking the React Fast Refresh. As @ryyppy raised:
"Fast Refresh requires a module to always export react components (nothing else), and a component function should have a name that starts with a (capitalized) letter A-Z."

We need to consider both cases.

  1. Multiple definitions of let bindings in a module, or using a submodule
  2. let default = ... without @react.component exported $$default

Can we make a sample project to see the breaking of the React Fast Refresh? Maybe we need to ask in the forum.

@cknitt
Copy link
Member Author

cknitt commented Oct 29, 2022

@mattdamon108 It is very easy to run into troubles with Fast Refresh not working. I can create a sample project with various cases. 🙂

@mununki
Copy link
Member

mununki commented Oct 29, 2022

Wow! it would be great.

@cknitt
Copy link
Member Author

cknitt commented Oct 29, 2022

But maybe we should open a separate issue for the fast refresh and default export things and close this one? As the original problem reported here is already resolved with the shared props type feature.

@stale
Copy link

stale bot commented May 28, 2023

The rescript-lang/syntax repo is obsolete and will be archived soon. If this issue is still relevant, please reopen in the compiler repo (https://github.com/rescript-lang/rescript-compiler) or comment here to ask for it to be moved. Thank you for your contributions.

@stale stale bot added the stale label May 28, 2023
@stale stale bot closed this as completed Jun 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants