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

feat: enhance dynamic import with template argument #266

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Mutefish0
Copy link

@Mutefish0 Mutefish0 commented Aug 6, 2024

We now support dynamic imports with template arguments:

import("./a/${test}/.mod.ts")

This will include all files matching ./a/**/mod.ts.

The problem:

Many libraries that use Deno FFI require multiple files compiled for specific platforms. For example:

Library entry:

  • https://jsr.io/@scope/mylib/0.0.1/mod.ts
const lib = await import(`./ffi/${Deno.build.target}/ffi.ts`);

The files compiled for multiple platforms:

  • https://jsr.io/@scope/mylib/0.0.1/ffi/aarch64-apple-darwin/ffi.ts
  • https://jsr.io/@scope/mylib/0.0.1/ffi/x86_64-unknown-linux-gnu/ffi.ts
  • https://jsr.io/@scope/mylib/0.0.1/ffi/x86_64-pc-windows-msvc/ffi.ts
    ...

In our project:

// main.ts
import xxx from "jsr:@scope/mylib"
...

Then use deno compile:

deno compile --unstable-ffi  --include ... main.ts

Currently, we have to manually --include all the files regardless of the actual platform it will run on:

deno compile --unstable-ffi  --include https://jsr.io/@scope/mylib/0.0.1/ffi/aarch64-apple-darwin/ffi.ts --include https://jsr.io/@scope/mylib/0.0.1/ffi/x86_64-unknown-linux-gnu/ffi.ts main.ts
  • This is inconvenient, especially after updating jsr:@scope/mylib, as we have to carefully --include the correct version:
- --include https://jsr.io/@scope/mylib/0.0.1/ffi/aarch64-apple-darwin/ffi.ts
+ --include https://jsr.io/@scope/mylib/0.0.2/ffi/aarch64-apple-darwin/ffi.ts
  • It also increases the bundle size since we actually need only one of the files.

Another option is to statically import all the FFI files, as shown here:

截屏2024-08-06 下午6 37 22

it will be more complicated to maintain the library, and also will incrase the compiled bundle size.

Proposal

Analyze the expressions in the template argument in specific cases if they are known at compile time:

import(`./${Deno.build.target}/mod.ts`)

We can replace ${Deno.build.target} with correct target

import("./aarch64-apple-darwin/mod.ts")

which then become statically analyzable.

Proof of concept

I have created a patch and some tests for deno_graph which rely on this PR.

  run_test(
    "
  await import(`./${Deno.build.target}.ts`);
  ",
    vec![
      ("file:///dev/x86_64-unknown-linux-gnu.ts", ""),
      ("file:///dev/aarch64-unknown-linux-gnu.ts", ""),
      ("file:///dev/x86_64-pc-windows-msvc.ts", ""),
      ("file:///dev/x86_64-apple-darwin.ts", ""),
      ("file:///dev/aarch64-apple-darwin.ts", ""),
      ("https://dev/aarch64-apple-darwin.ts", ""),
    ],
    if cfg!(all(
      target_arch = "x86_64",
      target_os = "macos",
      target_vendor = "apple"
    )) {
      vec!["file:///dev/x86_64-apple-darwin.ts"]
    } else if cfg!(all(
      target_arch = "aarch64",
      target_os = "macos",
      target_vendor = "apple"
    )) {
      vec!["file:///dev/aarch64-apple-darwin.ts"]
    } else if cfg!(all(
      target_arch = "x86_64",
      target_os = "linux",
      target_env = "gnu"
    )) {
      vec!["file:///dev/x86_64-unknown-linux-gnu.ts"]
    } else if cfg!(all(
      target_arch = "aarch64",
      target_os = "linux",
      target_env = "gnu"
    )) {
      vec!["file:///dev/aarch64-unknown-linux-gnu.ts"]
    } else if cfg!(all(
      target_arch = "x86_64",
      target_vendor = "pc",
      target_os = "windows",
      target_env = "msvc"
    )) {
      vec!["file:///dev/x86_64-pc-windows-msvc.ts"]
    } else {
      vec![]
    },
  )
  .await;

run_test(
    "
  await import(`https://dev/${Deno.build.target}.ts`);
  ",
    vec![
      ("https://dev/x86_64-unknown-linux-gnu.ts", ""),
      ("https://dev/aarch64-unknown-linux-gnu.ts", ""),
      ("https://dev/x86_64-pc-windows-msvc.ts", ""),
      ("https://dev/x86_64-apple-darwin.ts", ""),
      ("https://dev/aarch64-apple-darwin.ts", ""),
    ],
    if cfg!(all(
      target_arch = "x86_64",
      target_os = "macos",
      target_vendor = "apple"
    )) {
      vec!["https://dev/x86_64-apple-darwin.ts"]
    } else if cfg!(all(
      target_arch = "aarch64",
      target_os = "macos",
      target_vendor = "apple"
    )) {
      vec!["https://dev/aarch64-apple-darwin.ts"]
    } else if cfg!(all(
      target_arch = "x86_64",
      target_os = "linux",
      target_env = "gnu"
    )) {
      vec!["https://dev/x86_64-unknown-linux-gnu.ts"]
    } else if cfg!(all(
      target_arch = "aarch64",
      target_os = "linux",
      target_env = "gnu"
    )) {
      vec!["https://dev/aarch64-unknown-linux-gnu.ts"]
    } else if cfg!(all(
      target_arch = "x86_64",
      target_vendor = "pc",
      target_os = "windows",
      target_env = "msvc"
    )) {
      vec!["https://dev/x86_64-pc-windows-msvc.ts"]
    } else {
      vec![]
    },
  )
  .await;

Related Issues

denoland/deno#24871

Works fine with my local patch:

DENORT_BIN=./target/debug/denort ./target/debug/deno compile --unstable-ffi -o ./main main.ts 

@CLAassistant
Copy link

CLAassistant commented Aug 6, 2024

CLA assistant check
All committers have signed the CLA.

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.

2 participants