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: expose eval method #289

Closed
wants to merge 3 commits into from
Closed

feat: expose eval method #289

wants to merge 3 commits into from

Conversation

geieredgar
Copy link
Contributor

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Documentation
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
This allows executing javascript code without having to use the mpsc::channel and helps to simplify handle_event_loop in tauri-runtime-wry (see tauri-apps/tauri#1962).

@wusyong
Copy link
Member

wusyong commented Jun 7, 2021

The problem is this method is not thread-safe. It should be called on the main thread only.
Mpsc channel could help you send the script on any thread.
I think one workaround is we could call evaluate_script immediately after dispatch_script if it's on main thread (ie. in the event loop)
@lucasfernog wdyt?

@geieredgar
Copy link
Contributor Author

Yes, calling evaluate_script immediately after dispatch_script is also what I did in my PR tauri-apps/tauri#1963 to fix tauri-apps/tauri#1962. I just think that solution is a bit ugly, which is why I opened this PR. In my opinion it would be a lot cleaner if wry did only provide the eval method and no evaluate_script, dispatch_script and Dispatcher, with a doc comment stating that eval should only be called on the main thread (I think this information should also be added to evaluate_script) and maybe a hint that users could use channels if they want to send scripts from any thread. I actually think that using the EventLoopProxy to send scripts from any thread is a better solution than using an mpsc channel because it always wakes up the event loop, which would not happen if a script is send via Dispatcher, where we cannot call evaluate_script afterwards and would have to send a message to the event loop anyway.

@wusyong
Copy link
Member

wusyong commented Jun 7, 2021

Yeah, I think we could do that. If @lucasfernog also agrees (since tauri will need some custom event on EventLoopProxy), let's remove the dispatcher and all its related methods and expose this instead.

@geieredgar
Copy link
Contributor Author

FYI, tauri already uses a WebviewMessage::EvaluateScript custom event and AFAICT doesn't use the Dispatcher.

@geieredgar
Copy link
Contributor Author

Closed in favor of #291.

@geieredgar geieredgar closed this Jun 8, 2021
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