-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Cleanup JS api types #3131
base: v3-alpha
Are you sure you want to change the base?
Cleanup JS api types #3131
Conversation
|
Name | Link |
---|---|
🔨 Latest commit | df01313 |
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 X ? TipsChat with CodeRabbit Bot (
|
*/ | ||
Center: () => void wails.Window.Center(), | ||
/** | ||
* Set the window title. | ||
* @param title | ||
* @param {string} title | ||
* @returns {Promise<void>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe void
will mean nothing is returned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the function that it calls also returns Promise<void>
. (https://github.com/jpatters/wails/blob/js-api-types/v3/internal/runtime/desktop/window.js#L65).
And when I followed it up the chain, it ended up here: https://github.com/jpatters/wails/blob/js-api-types/v3/internal/runtime/desktop/runtime.js#L84
Is this incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the call to the backend returns a promise but the void
here ignores it and returns nothing at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Happy to update. What is the desired behaviour? I would have expected a promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing to wait for as there's nothing being returned so it works like a normal synchronous function I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Is there an opportunity for the request to the backend to fail though? Should the promise propagate up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so but we could make promise a standard regardless. Just had PRs in the past to remove that when they don't return anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. If there's no opportunity for the request from the backend to be rejected then that makes sense for sure. I'll dig through some more code to see what might happen on the backend and come back with a stronger opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you rightly said, those wrappers are out of date so they very well may have changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the backend does return http errors when things go wrong and the response handler does properly call reject when the http code is not in the 200 range. So I would suggest not swallowing those errors and allowing the promise to propagate up to the caller. It will require a few more changes for this to all behave as expected as the method that this calls also swallows the rejection. But minor changes. If this sounds reasonable, I will make the changes in this pr.
Please could you update the changelog located at |
Ah. Good to know. Thanks. |
9ea5b29
to
df01313
Compare
@jpatters - A lot has gone on since this PR was open. Let's discuss on discord 🙏 |
Yeah, for sure |
Description
The adds better type support for the js apis. It also makes the exports from the npm package more conventional. Previously usage looked like this:
usage now looks like this
Because of the import change, I have marked this as a breaking change however, the documentation in the readme for the js package already described it as working this way even though it did not.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration using
wails doctor
.Test Configuration
Please paste the output of
wails doctor
. If you are unable to run this command, please describe your environment in as much detail as possible.Checklist:
website/src/pages/changelog.mdx
with details of this PR