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.

Sign up for GitHub

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

Jump to bottom

feat: rewrite to use Deno FFI #99

Merged
merged 7 commits into from
Jan 26, 2022
Merged

feat: rewrite to use Deno FFI #99

merged 7 commits into from
Jan 26, 2022

Conversation

littledivy
Copy link
Contributor

@littledivy littledivy commented Dec 20, 2021 •
edited
Loading

Breaking Changes

  • Synchronous API.
    • Most of the calls get offloaded to the web renderer anyways. Also probably don't want to go through the overhead of spawning a tokio green thread in non blocking FFI.
    • Note that this does not block the JS event loop but spawning multiple webviews (one at a time) is still possible. This is a design limitation. One solution to this is native support for FFI callbacks (WIP)
  • Leverage JS setters
    • setTitle changes to a setter on webview.title. Same for setSize

cc @eliassjogreen

Closes #96
Closes #98
Closes #94
Closes #6
Closes #76
Closes #89
Closes #79

@littledivy littledivy changed the title rewrite to use Deno FFI [wip] rewrite to use Deno FFI Dec 20, 2021
@littledivy
Copy link
Contributor Author

One final blocker denoland/deno#13162

@brandonros
Copy link

might want to update the README to disclose that you got rid of the need for https://github.com/webview/webview_deno#deno https://github.com/denosaurs/plug

@brandonros brandonros mentioned this pull request Dec 27, 2021
Closed
@eliassjogreen
Copy link
Member

@brandonros Plug will still be needed for this pr as to allow for versioned caching of dylibs and easy loading from urls.

brandonros
brandonros reviewed Dec 27, 2021
View reviewed changes
src/ffi.ts Outdated Show resolved Hide resolved
@eliassjogreen eliassjogreen mentioned this pull request Jan 8, 2022
Closed
justinmchase
justinmchase reviewed Jan 10, 2022
View reviewed changes
examples/external.ts Outdated Show resolved Hide resolved
examples/multiple.ts Show resolved Hide resolved
examples/remote.ts Show resolved Hide resolved
src/webview.ts Show resolved Hide resolved
src/webview.ts Outdated Show resolved Hide resolved
@littledivy
Copy link
Contributor Author

littledivy commented Jan 10, 2022 •
edited
Loading

@eliassjogreen Callback support in Deno FFI is blocked on rusty_v8's Locker API. I had a small discussion internally and we (Deno) are not so sure about the concept of Rust calling into V8.

To unblock deno_webview, maybe we should just do a wrapper to the callbacks? This increases maintenance but was already the case with plugins. WDYT?

@eliassjogreen
Copy link
Member

Sure, that sounds like a good idea until callback support is added.

@nnmrts
Copy link

nnmrts commented Jan 13, 2022

Sorry for going a bit off-topic but I just wanted to say I added a small bounty for issue #96 (which should be closed by this PR) to express my excitement and gratefulness. Thanks for the work y'all put in so far. I would try to help if I had any knowledge of rust or this FFI stuff. 😝

@littledivy littledivy marked this pull request as ready for review January 14, 2022 04:40
@littledivy littledivy changed the title [wip] rewrite to use Deno FFI feat: rewrite to use Deno FFI Jan 14, 2022
eliassjogreen
eliassjogreen requested changes Jan 14, 2022
View reviewed changes
src/ffi.ts Outdated Show resolved Hide resolved
@eliassjogreen
Copy link
Member

Could you also update the version tag in Cargo.toml

@brandonros
Copy link

you might want to delete the empty .gitmodules leftover file

@brandonros
Copy link

@eliassjogreen anything left review wise?

@eliassjogreen eliassjogreen merged commit a91588e into webview:main Jan 26, 2022
@brandonros
Copy link

brandonros commented Jan 27, 2022 •
edited
Loading

Running the README example:

import { Webview } from "https://deno.land/x/webview/mod.ts";

const html = `
  <html>
  <body>
    <h1>Hello from deno v${Deno.version.deno}</h1>
  </body>
  </html>
`;

const webview = new Webview();
webview.navigate(`data:text/html,${encodeURIComponent(html)}`);
webview.run();
error: TS2571 [ERROR]: Object is of type 'unknown'.
    sys.symbols.deno_webview_get_recv().then((recv) => {
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    at https://deno.land/x/[email protected]/src/webview.ts:62:5

TS7006 [ERROR]: Parameter 'recv' implicitly has an 'any' type.
    sys.symbols.deno_webview_get_recv().then((recv) => {
                                              ~~~~
    at https://deno.land/x/[email protected]/src/webview.ts:62:47

Found 2 errors.

Not sure if you need to release?

Edit: needed deno v1.18.0

@eliassjogreen
Copy link
Member

Strange i am not getting the same error when running locally, have you tried the --reload flag? could be some cache issue

@littledivy littledivy deleted the rewrite_ffi branch January 27, 2022 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmchase justinmchase justinmchase left review comments

@Miaourt Miaourt Miaourt left review comments

@brandonros brandonros brandonros left review comments

@eliassjogreen eliassjogreen eliassjogreen requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
6 participants
@littledivy @brandonros @eliassjogreen @nnmrts @justinmchase @Miaourt