-
Notifications
You must be signed in to change notification settings - Fork 464
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
Conversation
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)
In this case, But // 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 /> |
There was a problem hiding this 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.
There was a problem hiding this 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.
|
There was a problem hiding this 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.
@zth What branch would you like this PR to be merged to? I'm going to add the changelog accordingly. |
@mununki go for the v11 one so it'll make it into 11.1. |
ac1bd47
to
5044784
Compare
Rebased, and change log added. |
It's a side-effect of #6429. I guess it is ppx-related issue, investigating |
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:
Lam_primitive.Pjs_call
representation