[v3] Update and fix runtime JS API#3295
Conversation
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Commit
|
Commit
|
Commit
|
|
Really appreciate you taking this on 🙏 On the whole it looks fine. The reason we removed the runtime from being embedded by default was to give developers the flexibility to choose whether they wanted the entire runtime loaded or pick and choose what they wanted from the npm module. I did consider embedding the compiled runtime if it wasn't production but then that'll create inconsistent behaviour between prod and non-prod. In this PR, I believe both debug and non debug runtimes will be included in every build? My approach was going to create an asset handler that embeds the compiled runtimes that could be used for examples or anyone who wants the full bundle. If the compiled runtime is bundled each time then developers lose the benefit of the npm module. Thoughts? |
Commit
|
I think I get your point (but please tell me if not). Having a separate asset handler should mean that the optimizer can drop the embedded runtime when it's not used. I'd be happy to work on this, but I can't see clearly what kind of API you have in mind. To be specific, how should it be plugged into the server pipeline? As middleware? As a different kind of asset server with its own constructor function? As a wrapper for the default asset server? Surely it cannot be passed as an option to the default asset server constructor, as that would most probably prevent dead code elimination from kicking in. Should it reside in its own separate package? If we can work out the details here (or on discord, as you prefer) I will get to work on it. |
|
BTW, what do you think of the npm package not populating the |
|
A further comment about the embedded runtime. In my discussion of the related commit I was proposing as a future change to embed and maybe even inject automatically the private runtime API (which is quite minimal), and leave the public API to the npm package, plus maybe an optional asset handler for examples and vanilla apps. I think this would be the best approach overall, but it requires some more effort and I don't think it fits in this PR. The asset handler you propose, on the other hand, would be a nice temporary solution. I was thinking that this could actually be best implemented as a middleware residing in a separate package; thus one might easily inject it into their own middleware chain, and the whole package would never be included in the binary if unused. |
Commit
|
Commit
|
|
This is pretty much it, all other commits are just maintenance operations. It's a lot of stuff, don't worry and take your time to answer, and as always thank you for your work! Summarising, apart from any comments you might have, I'd like to get your feedback about the following three points: |
|
Just as a reference if you probably start working on that one, all code is already there for injection in v2: wails/v2/pkg/assetserver/assetserver.go Lines 140 to 181 in c5381dc So you don't have to start all over :) |
| // Add the method to the map | ||
| result.set(method, targetWindow[method]); | ||
| } | ||
| export function Boot() { |
There was a problem hiding this comment.
What do you think about calling this Enable?
There was a problem hiding this comment.
Sounds better to me too, will rename it.
|
Thanks for taking the time to go through all this! On the whole I think this is a great step forward. I'll be moving the compiled runtime into its own FS handler so it can be better used rather than having copies of |
1813410 to
839e42e
Compare
|
@fbbdev - apologies! As part of fixing the merge conflicts I've appeared to created a bad merge. I did push up a reset but that does not appear to have worked as intended. Could you please force push your branch again? Thanks 😬 |
leaanthony
left a comment
There was a problem hiding this comment.
This is incredible! Thank you 🙏
| ### Changed | ||
|
|
||
| <<<<<<< HEAD | ||
| ======= |
* Cleanup bundled runtime entry point * Fix JS representation of Screen struct * Expose IsFocused method in Window interface * Update JS window API * Fix cleanup of WML listeners * Bundle runtime as ES module * Update runtime dependencies * Update runtime types and events * Update bundled runtime * Update changelog --------- Co-authored-by: Lea Anthony <lea.anthony@gmail.com>
Fixes #4489 ## Problem The npm package @wailsio/runtime doesn't work for drag-and-drop because Go backend calls window.wails.Window.HandlePlatformFileDrop(), but the npm package doesn't populate window.wails (by design for encapsulation). ## Root Cause PR #3295 (March 2024) intentionally removed window.wails assignment from the npm package to improve encapsulation. However, this broke platform handlers that Go backend relies on. ## Solution Move HandlePlatformFileDrop from public API (window.wails) to internal API (window._wails), following the existing pattern: - window._wails.invoke - window._wails.environment - window._wails.flags ## Changes - Register handlePlatformFileDrop in window._wails namespace - Update Go backend to call window._wails.handlePlatformFileDrop() - Use camelCase naming for consistency with other internal API methods - Rebuild bundled runtime with changes ## Benefits ✅ npm package works without window.wails pollution ✅ Maintains encapsulation of public API ✅ Platform handlers clearly separated as internal ✅ Follows existing internal API conventions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
* fix: Move HandlePlatformFileDrop to window._wails internal API Fixes #4489 ## Problem The npm package @wailsio/runtime doesn't work for drag-and-drop because Go backend calls window.wails.Window.HandlePlatformFileDrop(), but the npm package doesn't populate window.wails (by design for encapsulation). ## Root Cause PR #3295 (March 2024) intentionally removed window.wails assignment from the npm package to improve encapsulation. However, this broke platform handlers that Go backend relies on. ## Solution Move HandlePlatformFileDrop from public API (window.wails) to internal API (window._wails), following the existing pattern: - window._wails.invoke - window._wails.environment - window._wails.flags ## Changes - Register handlePlatformFileDrop in window._wails namespace - Update Go backend to call window._wails.handlePlatformFileDrop() - Use camelCase naming for consistency with other internal API methods - Rebuild bundled runtime with changes ## Benefits ✅ npm package works without window.wails pollution ✅ Maintains encapsulation of public API ✅ Platform handlers clearly separated as internal ✅ Follows existing internal API conventions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * chore: Add changelog entry for drag-and-drop fix Updates UNRELEASED_CHANGELOG.md with the fix for #4489. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
Description
The main purpose of this PR is to bring the JS Window API up to date wrt the current WebviewWindow interface, as briefly discussed some time ago in the wails3 discord channel. In addition, it brings some bug fixes, code cleanups and quality-of-life improvements to the JS runtime. I decided to pack everything into a single PR because many changes overlap and I wanted to avoid a cascade of merge conflicts.
As a result this PR is somewhat large. In order to facilitate the review process, I divided it into many logically independent commits. I am gonna explain the changes in detail below through separate comments on a per-commit basis.
I am opening the PR against the
v3-alphabranch. Should this PR be merged, I am willing to take care of any potential merge conflicts with thev3-alpha-linuxbranch.Here follows a summary of the changes, some of which are breaking:
@wailsio/runtimepackage does not export the API to thewindow.wailsobject anymore, and does not start the WML system. The bundledruntime.jsstill does both.Screenstruct has been updated to match its Go counterpart.WebviewWindow.IsFocusedmethod has been exposed on theWindowinterface.Windowinterface. This change is breaking because some methods have changed name and/or return type (although the most commonly used ones remain unchanged).@wailsio/runtime/src/window) now exports the containing window object as a default export. It is not possible anymore to use ESM named import or namespace import syntax.@wailsio/runtimepackage. The benefits being that it can be easily imported into other modules and the ESM import system ensures it is loaded at most once. The change is breaking because thetype="module"attribute must now be added to all script tags loading it.As regards OS compatibility,
Because of change no. 5, this PR supersedes #3280
I am gonna push the updated changelog ASAP.
Type of change
How Has This Been Tested?
Before testing the PR code, I recommend running
go clean ./...in thev3folder to ensure the build system picks up all changes.Most examples have been brought up to date and thoroughly tested. The only exceptions are the
buildexample (not relevant to the changes), thedevexample (it is incomplete and probably not relevant), thedrag-n-dropexample (it has been updated but not tested as it doesn't work on macOS anyways).The
menuandpluginexamples are not working with my test configuration, but the problems do not appear to originate with the new changes.Test Configuration
Checklist: