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

RFC: conditional module FFI #6946

Open
Tracked by #6210
cometkim opened this issue Aug 11, 2024 · 15 comments
Open
Tracked by #6210

RFC: conditional module FFI #6946

cometkim opened this issue Aug 11, 2024 · 15 comments
Milestone

Comments

@cometkim
Copy link
Member

cometkim commented Aug 11, 2024

Background

Imagine a class statement written from external JS.

// rescript_error.mjs
export class RescriptError extends Error {
  constructor(exnId, payload) {
    super();
  }
}

// and in rescript_error.cjs
module.exports = {
  RescriptError,
};

This is a pretty common situation because ReScript doesn't support ES classes.

However, ReScript's current FFI approach cannot select the appropriate resolution by its syntax.

type t

@module("./rescript_error.mjs") @new external makeErrorEsm: unit => t = "RescriptError"
@module("./rescript_error.cjs") @new external makeErrorCjs: unit => t = "RescriptError"

// and it might need a runtime like UMD

This can be solved today using bundlers' module alias or Node.js' conditional exports/imports resolution, but these are all platform-dependent features.

{
  "imports": {
    "#error": {
      "import": "./rescript_error.mjs",
      "require": "./rescript_error.cjs"
    }
  }
}

The toolchain must be able to produce output that is appropriate for its target. That's true even for FFIs.

Suggested Solution

There are currently two module specs: esmoudle and commonjs (a composite of the two, dual, will be added in the future #6209).

Users can specify either of these two as target in the @module statement.

type t

@module({ target: "esmodule", from: "./rescript_error.mjs" })
@module({ target: "commonjs", from: "./rescript_error.cjs" })
@new external makeError: unit => t = "RescriptError"

The library may not support all conditions. The compiler should raise an error if it cannot find a target that matches a module in the project.

If target omitted, the * pattern is implicitly used.

@module("./rescript_error.ts")
// is equivalent
@module({ target: "*", from: "./rescript_error.ts" })
@zth
Copy link
Collaborator

zth commented Aug 11, 2024

Yes, that's a great catch and would be very useful. I'm all for it 👍

@cometkim cometkim added this to the v12 milestone Aug 11, 2024
@cknitt
Copy link
Member

cknitt commented Aug 11, 2024

This is great! I am not sure I like the name "cond" though.

@zth
Copy link
Collaborator

zth commented Aug 11, 2024

Maybe we should call it the same as we are calling it in rescript.json?

@cometkim
Copy link
Member Author

cometkim commented Aug 11, 2024

Maybe we should call it the same as we are calling it in rescript.json?

That's "module" (the format), already duplicated with @module keyword. And If we use the "module" here, we should use something like module_ to avoid keyword collisions.

"cond" is inspired by Node.js' and bundlers' "conditions"

Some tools does something similar with "env" or "mode". It's basically a flag to customize full config.

Rust has "target", which is probably the most common way to express compilation conditions.

@cometkim
Copy link
Member Author

cometkim commented Aug 11, 2024

The @module resolution must be selected for all conditions. If omitted, the * pattern is implicitly used.

This should be relaxed based on the actual project configuration. Then implementation is a bit more complex, but it helps to maintain forward compatibility with future targets.

@cometkim
Copy link
Member Author

@zth @cknitt Does the last change make sense?

image

@zth
Copy link
Collaborator

zth commented Aug 11, 2024

Looks good to me!

@DZakh
Copy link
Member

DZakh commented Aug 12, 2024

I think for the example with RescriptError what we actually need is a feature for @module to resolve relative path to the compiled ReScript output.

@DZakh
Copy link
Member

DZakh commented Aug 12, 2024

But the feature is also very nice and will be useful in any way

@cknitt
Copy link
Member

cknitt commented Aug 12, 2024

Or what about type instead of target or cond?

@module({ type: "esmodule", from: "./rescript_error.mjs" })

@cometkim
Copy link
Member Author

Personally, I don't prefer to call the "targeted module format" config as just "type"

And it might be confusing with the standard "type" attribute.

@module({ type_: "esmodule", with: { type_: "json", from: "./file.json" } })
@val external content: Json.t = "default"

@cknitt
Copy link
Member

cknitt commented Aug 12, 2024

Ok, and what about format?

@module({ format: "esmodule", from: "./rescript_error.mjs" })

?

@cometkim
Copy link
Member Author

Ok, and what about format?

I think that it's good if its mental model is the format of the from source, not to tailor the compilation output.

@DZakh
Copy link
Member

DZakh commented Aug 12, 2024

I like target

@cknitt
Copy link
Member

cknitt commented Aug 13, 2024

Ok, seems people like target most. Fine with me too! 👍

@cknitt cknitt moved this to RFC in ReScript development Sep 9, 2024
@cknitt cknitt moved this to Todo in RFCs Nov 28, 2024
@cknitt cknitt added this to RFCs Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Archived in project
Development

No branches or pull requests

4 participants