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

PPX proposal: React.lazy #498

Open
bloodyowl opened this issue Nov 27, 2019 · 5 comments
Open

PPX proposal: React.lazy #498

bloodyowl opened this issue Nov 27, 2019 · 5 comments
Labels
ppx issues related to the reason-react ppx RFC

Comments

@bloodyowl
Copy link
Contributor

bloodyowl commented Nov 27, 2019

it's currently pretty hard to get a type-safe dynamic import using BuckleScript. it's even harder to use React.lazy.

how would you feel about having a PPX:

module ReactLazy = {
  [@bs.module "react"]
  external make:
    (unit => Js.Promise.t({. "default": React.component('a)})) =>
    React.component('a) =
    "lazy";
};

[@react.lazy]
module MyComponentLazy = MyComponent;

// that would expand to something like:
module MyComponentLazy: MyComponentType = {
  module type MyComponentType = (module type of MyComponent);
  [@bs.val] external myComponent: (module MyComponentType) = "undefined";
  include (val myComponent);
  [@bs.val]
  external import:
    ([@bs.as "MODULE_RELATIVE_PATH"] _, unit) => Js.Promise.t(module MyComponentType) =
    "import";
  let make =
    ReactLazy.make(() =>
      import()
      |> Js.Promise.then_((module MyComponent: MyComponentType) =>
           Js.Promise.resolve({"default": MyComponent.make})
         )
    );
};
@ozanmakes
Copy link

I haven't had the chance to use React.lazy or React.Suspense yet, and haven't tested the following code, but I gave it a shot at approximating your example transformation with reason-macros:

// MyComponent.re
[@react.component]
let make = (~title=?, ~children) => <div ?title> children </div>;
// MyComponentLazy.re
%react.lazy
(MyComponent, "./MyComponent.bs.js");
// Shared.macros.re
[@macro.name "react.lazy"]
let%macro.toplevel _ =
  (moduleName: capIdent, modulePath: string) => [%str
    include (
      {
        [@bs.val]
        external import:
          ([@bs.as "$eval{modulePath}"] _, unit) => Js.Promise.t('a) =
          "import";

        [@bs.module "react"]
        external lazy_: (unit => Js.Promise.t('a)) => 'a = "lazy";

        let makeProps = Eval__moduleName.makeProps;
        let make =
          lazy_(() =>
            import()
            |> Js.Promise.then_(x =>
                 Js.Promise.resolve({"default": x##make}) |> Obj.magic
               )
          );
      }:
        (module type of {
        let makeProps = Eval__moduleName.makeProps;
        let make = Eval__moduleName.make;
      })
            )
  ];
// Generated MyComponentLazy module type

{
  let makeProps:
    (~title: 'a=?, ~children: 'b, ~key: string=?, unit) =>
    {. "children": 'b, "title": option('a)};
  let make:
    {. "children": React.element, "title": option(string)} => React.element;
}
// Generated MyComponentLazy.bs.js

// Generated by BUCKLESCRIPT, PLEASE EDIT WITH CARE

import * as React from "react";
import * as Caml_option from "../node_modules/bs-platform/lib/es6/caml_option.js";

function makeProps(prim, prim$1, prim$2, prim$3) {
  var tmp = {
    children: prim$1
  };
  if (prim !== undefined) {
    tmp.title = Caml_option.valFromOption(prim);
  }
  if (prim$2 !== undefined) {
    tmp.key = Caml_option.valFromOption(prim$2);
  }
  return tmp;
}

var make = React.lazy((function (param) {
        return import("./MyComponent.bs.js").then((function (x) {
                      return Promise.resolve({
                                  default: x.make
                                });
                    }));
      }));

export {
  makeProps ,
  make ,
  
}
/* make Not a pure module */

Would've been easier to do (and also allow us to keep makeProps as external) if reason-macros transformed module names on the right hand side. For example, module type Foo = module type of Eval__name or include Eval__name. But I think something like this could do the trick until this issue is resolved.

@rickyvetter
Copy link
Contributor

I like this concept but we can't implement it until we get a reliable and typesafe way to define MODULE_RELATIVE_PATH. If we have a way to do this then it's worth building!

@bloodyowl
Copy link
Contributor Author

@bobzhang would that be hard to implement in BuckleScript?

@peterpme peterpme added the RFC label Apr 22, 2020
@rickyvetter
Copy link
Contributor

rickyvetter commented May 5, 2020

BTW done a bit more work to research this and I want to document the three things I believe we need from BuckleScript:

  1. A function that goes from MyModule to Js.Import.t((module typeof MyModule)). Suggested sytax: [%import MyModule]. This function should compile-time error if you pass a module that isn't a static, file-level construct which has 2:
  2. We need an annotation for files to opt in to dynamic imports. This might look like [@allowDynamicImports] at the top module level. This is because a number of compiler optimizations need to be disabled in these modules. [@bs.module] external foo: int = "myModule"' needs to be converted to a concrete value in the module. let foo = MyOtherModule.foo;' needs to never be short circuited at callsites to avoid statically importing these files instead of using through the dynamic import process.
  3. A function external import: t('module) => Js.Promise.t('module)

Two might seem odd given the way dynamic imports in JS work today, but I think it's actually an improvement as a module author you have more control over how this module is used. This might be able to be done without the annotation, but I think it would be much harder and I am not sure the tradeoffs are worth it.

@rickyvetter rickyvetter added the ppx issues related to the reason-react ppx label May 11, 2020
@zth
Copy link

zth commented May 13, 2020

@rickyvetter thanks for sharing your research, this is really exciting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ppx issues related to the reason-react ppx RFC
Projects
None yet
Development

No branches or pull requests

5 participants