Skip to content

Fix making the static import for the dynamic import of external ffi #6664

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 4 commits into from
Mar 5, 2024

Conversation

mununki
Copy link
Member

@mununki mununki commented Mar 4, 2024

Fixes #6663

This PR fixes the issue of making the static import(require) for the dynamic import of external ffi. The main changes are as follows:

  1. Lam_primitive.Pjs_call representation
| Pjs_call of {
    (* Location.t *  [loc] is passed down *)
    prim_name : string;
    arg_types : External_arg_spec.params;
    ffi : External_ffi_types.external_spec;
+   dynamic_import: bool;
  }

@mununki mununki requested review from zth, cristianoc and tsnobip March 4, 2024 09:34
@mununki
Copy link
Member Author

mununki commented Mar 4, 2024

One thing I haven't implemented yet. In case of the external binding to the function, it throws the build error.

// A.res
@module("lib")
external f: unit => string = "default"

// B.res
let _ = Js.import(A.f)
Invalid argument: Dynamic import requires a module or module value that is a file as argument. Passing a value or local module is not allowed.

In this case, A.f is converted to J.expression_desc=J.Fun that doesn't have an information about module_id instead of J.Var.

But @react.component use-case would be fine.

// A.res
@module("lib") @react.component
external make: unit => React.element = "default"

// B.res
module C = {
  let make = React.lazy_(() => Js.import(A.make))
}

<C />

Copy link
Member

@tsnobip tsnobip left a comment

Choose a reason for hiding this comment

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

I have little knowledge about dynamic import but the PR looks fine to me, the tests are clear. The changes just feel a bit too pervasive but I don't see a way to avoid this. Maybe @cristianoc has some ideas.

Copy link
Collaborator

@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.

1 where is dynamic_import set? Can't see it being set in the changed code. Is it the case that it was set already before, but now it's just passed around?

2 what happens when the same value is also used as a normal value, not just a dynamic import? I guess the scenario is not desirable, but I wonder whether it would currently leas to accessing an undefined value as the static import has been removed.

@mununki
Copy link
Member Author

mununki commented Mar 5, 2024

1 where is dynamic_import set? Can't see it being set in the changed code. Is it the case that it was set already before, but now it's just passed around?

2 what happens when the same value is also used as a normal value, not just a dynamic import? I guess the scenario is not desirable, but I wonder whether it would currently leas to accessing an undefined value as the static import has been removed.

  1. This changed code is to check whether the called external ffi is dynamic import, then it omits the ffi dependency when the external ffi dependency is added as a static import. It is happened inside lam_compile_env.ml using cached_tbl. I think passing the dynamic_import as an argument in the lambda converting functions would be more clear and avoiding side effect instead of making a state to set on/off.
  2. It doesn't affect using a normal value with the dyanimc importing at the same module due to the implementation above. I've added the test case for that ac1bd47

Copy link
Collaborator

@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.

Great.
Would you add a changelog?
Not sure why Mac tests started to time out, whether it's related or not.

@mununki
Copy link
Member Author

mununki commented Mar 5, 2024

@zth What branch would you like this PR to be merged to? I'm going to add the changelog accordingly.

@zth
Copy link
Collaborator

zth commented Mar 5, 2024

@mununki go for the v11 one so it'll make it into 11.1.

@mununki mununki force-pushed the fix-dynamic-import-external branch from ac1bd47 to 5044784 Compare March 5, 2024 10:16
@mununki mununki changed the base branch from master to 11.0_release March 5, 2024 10:16
@mununki
Copy link
Member Author

mununki commented Mar 5, 2024

Rebased, and change log added.

@zth zth merged commit 9a779e9 into 11.0_release Mar 5, 2024
@zth zth deleted the fix-dynamic-import-external branch March 5, 2024 10:34
@cometkim
Copy link
Member

cometkim commented Mar 5, 2024

Not sure why Mac tests started to time out, whether it's related or not.

It's a side-effect of #6429. I guess it is ppx-related issue, investigating

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.

Dynamic import compiled to dynamic import + static import which hurts code splition
5 participants