Skip to content

Add React.lazy #95

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

Merged
merged 1 commit into from
May 2, 2023
Merged

Add React.lazy #95

merged 1 commit into from
May 2, 2023

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented May 2, 2023

This adds support for React.lazy.

Usage from ReScript is like this:

module DynamicallyImportedComponent = {
  let make = React.lazy_(() => Js.import(DynamicallyImportedComponent.make))
}

The binding is not zero cost because React.lazy expects a default export and we need to adapt to that.

Closes #81.

@cknitt cknitt requested review from cristianoc and mununki May 2, 2023 09:24
Copy link
Contributor

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments for my own understanding.

@@ -143,6 +143,13 @@ module Experimental = {
}
}

type dynamicallyImportedModule<'a> = {default: component<'a>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much context on this so will ask very basic questions.
Do we know the component will have default export? Is this a property that react lazy relies on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the shape that React.lazy expects.

type dynamicallyImportedModule<'a> = {default: component<'a>}

@module("react")
external lazy_: (unit => promise<dynamicallyImportedModule<'a>>) => component<'a> = "lazy"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the underscore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lazy is a reserved word unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right it's on the list.
Perhaps worth opening an issue to explore whether it absolutely needs to be a keyword.
We removed a few keywords successfully recently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lazy is a nice add-on, but I think it can be moved to the standard library rather than the language.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lazy is a nice add-on, but I think it can be moved to the standard library rather than the language.

If I understand correctly, this is a rescript-react repo, not compiler.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created an issue here: rescript-lang/rescript#6241

@mununki
Copy link
Member

mununki commented May 2, 2023

Can you share the js out of this example?

module DynamicallyImportedComponent = {
  let make = React.lazy_(() => Js.import(DynamicallyImportedComponent.make))
}

@module("react")
external lazy_: (unit => promise<dynamicallyImportedModule<'a>>) => component<'a> = "lazy"

let lazy_ = load => lazy_(async () => {default: await load()})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, do we need to expose this wrapper function? If not, would user usage be different?

Copy link
Member Author

@cknitt cknitt May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we didn't expose this wrapper function, the user would need to write

let make = React.lazy_(async () => {default: await Js.import(DynamicallyImportedComponent.make)})

instead of

let make = React.lazy_(() => Js.import(DynamicallyImportedComponent.make))

@cknitt
Copy link
Member Author

cknitt commented May 2, 2023

Can you share the js out of this example?

var make = React.lazy_(function () {
      return import("./DynamicallyImportedComponent.bs.mjs").then(function (m) {
                  return m.make;
                });
    });

LGTM!

@mununki
Copy link
Member

mununki commented May 2, 2023

Can you share the js out of this example?

var make = React.lazy_(function () {
      return import("./DynamicallyImportedComponent.bs.mjs").then(function (m) {
                  return m.make;
                });
    });

LGTM!

The signature is slightly different from TypeScript's, but does it work well at runtime?

@mununki
Copy link
Member

mununki commented May 2, 2023

Can you share the js out of this example?

var make = React.lazy_(function () {
      return import("./DynamicallyImportedComponent.bs.mjs").then(function (m) {
                  return m.make;
                });
    });

LGTM!

It seems like the default key defined as the dynamicallyImportedModule type has disappeared from the output, and I'm curious if it's okay at runtime.

@cknitt
Copy link
Member Author

cknitt commented May 2, 2023

It seems like the default key defined as the dynamicallyImportedModule type has disappeared from the output, and I'm curious if it's okay at runtime.

Yes, that's ok and it works at runtime.

The React.lazy_ that's invoked in the JS output is our wrapper function that adds the "default" wrapper.

@mununki
Copy link
Member

mununki commented May 2, 2023

It seems like the default key defined as the dynamicallyImportedModule type has disappeared from the output, and I'm curious if it's okay at runtime.

Yes, that's ok and it works at runtime.

The React.lazy_ that's invoked in the JS output is our wrapper function that adds the "default" wrapper.

Ah, understood. Thanks!

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 this pull request may close these issues.

Any plan to support React.lazy?
4 participants