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

Can we make opendal-nodejs running on browser or electron? #3887

Open
Xuanwo opened this issue Jan 1, 2024 · 14 comments
Open

Can we make opendal-nodejs running on browser or electron? #3887

Xuanwo opened this issue Jan 1, 2024 · 14 comments
Assignees

Comments

@Xuanwo
Copy link
Member

Xuanwo commented Jan 1, 2024

As requested by #3803 (comment), can we make merge wasm support with opendal-nodejs so that we can run on browser or electron?

For example, aws-crt is written in C, but also provide browser support: https://www.npmjs.com/package/aws-crt

Maybe via napi's native wasm support?

https://github.com/napi-rs/napi-rs/blob/03eb476cef61fb15f1ec071c415cbf6fbfa8e830/crates/napi/Cargo.toml#L78-L87

@Xuanwo
Copy link
Member Author

Xuanwo commented Jan 1, 2024

Invite @suyanhanx, @fyears, @Brooooooklyn to join in the discussion.

@Xuanwo
Copy link
Member Author

Xuanwo commented Jan 1, 2024

Current progress:

:( NAPI_TARGET=wasm32-unknown-unknown pnpm run build

> [email protected] build /home/xuanwo/Code/apache/incubator-opendal/bindings/nodejs
> napi build --platform --features "${NAPI_FEATURES:-}" --target "${NAPI_TARGET:-}" --release --js generated.js --dts generated.d.ts && node ./scripts/header.js
   ...
   Compiling opendal-nodejs v0.44.1 (/home/xuanwo/Code/apache/incubator-opendal/bindings/nodejs)
error: future cannot be sent between threads safely
   --> bindings/nodejs/src/lib.rs:772:1
    |
772 | #[napi]
    | ^^^^^^^ future created by async block is not `Send`
    |
    = help: the trait `std::marker::Send` is not implemented for `(dyn futures::Future<Output = (std::string::String, std::result::Result<opendal::Metadata, opendal::Error>)> + 'static)`
note: captured value is not `Send` because `&mut` references cannot be sent unless their referent is `Send`
   --> bindings/nodejs/src/lib.rs:772:1
    |
772 | #[napi]
    | ^^^^^^^ has type `&mut Lister` which is not `Send`, because `Lister` is not `Send`
note: required by a bound in `execute_tokio_future`
   --> /home/xuanwo/.cargo/registry/src/index.crates.io-6f17d22bba15001f/napi-2.14.1/src/tokio_runtime.rs:121:18
    |
119 | pub fn execute_tokio_future<
    |        -------------------- required by a bound in this function
120 |   Data: 'static + Send,
121 |   Fut: 'static + Send + Future<Output = Result<Data>>,
    |                  ^^^^ required by this bound in `execute_tokio_future`
    = note: this error originates in the attribute macro `napi` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `opendal-nodejs` (lib) due to previous error
(node:30467) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead.
(Use `node --trace-deprecation ...` to show where the warning was created)
Internal Error: Command failed: cargo build --release --target wasm32-unknown-unknown
    at genericNodeError (node:internal/errors:956:15)
    at wrappedFn (node:internal/errors:510:14)
    at checkExecSyncError (node:child_process:890:11)
    at Object.execSync (node:child_process:962:15)
    at BuildCommand.<anonymous> (/home/xuanwo/Code/apache/incubator-opendal/bindings/nodejs/node_modules/.pnpm/@[email protected]/node_modules/@napi-rs/cli/scripts/index.js:11555:30)
    at Generator.next (<anonymous>)
    at /home/xuanwo/Code/apache/incubator-opendal/bindings/nodejs/node_modules/.pnpm/@[email protected]/node_modules/@napi-rs/cli/scripts/index.js:3552:69
    at new Promise (<anonymous>)
    at __awaiter$1 (/home/xuanwo/Code/apache/incubator-opendal/bindings/nodejs/node_modules/.pnpm/@[email protected]/node_modules/@napi-rs/cli/scripts/index.js:3548:10)
    at BuildCommand.execute (/home/xuanwo/Code/apache/incubator-opendal/bindings/nodejs/node_modules/.pnpm/@[email protected]/node_modules/@napi-rs/cli/scripts/index.js:11325:16)
 ELIFECYCLE  Command failed with exit code 1.

@Brooooooklyn
Copy link

@Xuanwo you can assign this issue to me :) I'm working on wasm32-wasi support in NAPI-RS

@suyanhanx
Copy link
Member

@Xuanwo you can assign this issue to me :) I'm working on wasm32-wasi support in NAPI-RS

Thank you for participating in this!
If you need any further assistance or help, feel free to let us know.

@Zheaoli
Copy link
Member

Zheaoli commented Jan 2, 2024

@Xuanwo you can assign this issue to me :) I'm working on wasm32-wasi support in NAPI-RS

FYI, the opendal is not support wasm32-wasi yet, I'm trying to do a min support

@Xuanwo
Copy link
Member Author

Xuanwo commented Jan 2, 2024

Supporting wasm32-wasi is cool, but it seems unrelated to the goal of running in a browser. For example, supporting the use case like remotely-save

@Zheaoli
Copy link
Member

Zheaoli commented Jan 2, 2024

Supporting wasm32-wasi is cool, but it seems unrelated to the goal of running in a browser. For example, supporting the use case like remotely-save

If we just want to make the opendal useable in electron. I think it already be done

image

image

@Xuanwo
Copy link
Member Author

Xuanwo commented Jan 2, 2024

If we just want to make the opendal useable in electron. I think it already be done

It seems more complex while on mobile as described in https://github.com/remotely-save/remotely-save/blob/master/docs/browser_env.md

@Zheaoli
Copy link
Member

Zheaoli commented Jan 2, 2024

It seems more complex while on mobile as described in

The mobile is totally different with the desktop. All of the hybrid mobile solutions is depend on the webview which is provided by the platform. Which is means that we need solve issue case by case(It's already not a issue related with the browser or electron)

If we just want to run opendal on the capacitorjs which is used by the plugin you mentioned, I might have some trick to run the workflow as normal.

@fyears
Copy link

fyears commented Jan 2, 2024

electron is just like nodejs and should work by calling nodejs library. browser is different.

If we just want to run opendal on the capacitorjs

no need to consider that. let’s focus on the general browser environment firstly

@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 18, 2024

Hi, @fyears, I'm now using remotely-save! Fantastic job! Let's see how we can improve it by using opendal.

@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 18, 2024

Following the discussion at napi-rs/napi-rs#1794, it seems we just need to wait for napi 3.0 and add wasm32 target during release.

@Xuanwo
Copy link
Member Author

Xuanwo commented Feb 20, 2024

cc @suyanhanx, after this implemented, we will need to rename bindings/nodejs to bindings/js?

@suyanhanx
Copy link
Member

cc @suyanhanx, after this implemented, we will need to rename bindings/nodejs to bindings/js?

I think it's okay to rename.

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

No branches or pull requests

5 participants