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(api): add abstractions to updater and window event listeners #4569

Merged
merged 7 commits into from
Jul 5, 2022

Conversation

lucasfernog
Copy link
Member

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@lucasfernog lucasfernog requested a review from a team as a code owner July 2, 2022 12:40
@lucasfernog
Copy link
Member Author

@FabianLars is this what you had in mind? :P

@FabianLars
Copy link
Member

Well initially i just wanted better docs, but you had me intrigued with this idea and you were right, this is indeed wayyy better, especially for typescript users because they don't have to handle the types themselves with this. I like it! 👍

I liked that it used to be uncoupled from appWindow tho, but i guess that's a fair trade-off (and it's not like you can't use the old way anymore)

@lucasfernog
Copy link
Member Author

what do you mean by uncoupled from appWindow @FabianLars ? you still needed it to call .listen.

@lucasfernog
Copy link
Member Author

@JonasKruckenberg I've followed #3731 here, anything you want to change?

@FabianLars
Copy link
Member

FabianLars commented Jul 4, 2022

what do you mean by uncoupled from appWindow @FabianLars ? you still needed it to call .listen.

For me it's just listen > appWindow.listen, that's all lol.
For users that use their frontend in both tauri and a normal browser appWindow likes to cause problems, but whatever. It's not that big of a deal and you still can listen to the events manually.
All good, just wanted to mention it for whatever reason sorry 🤷

Edit: Oh another "side-effect" that somewhat came up on discord is that this abstraction makes it less obvious that you have to unlisten when the component unmounts (at least in React)

FabianLars
FabianLars previously approved these changes Jul 4, 2022
@lucasfernog
Copy link
Member Author

From an API perspective the events are tied to the appWindow though, so they'll have to be a little smart with the bundler config to only import appWindow for tauri builds - or inject dummy values.

Why does it make it "less obvious"? You're still just listening to an event - and it still returns the unlisten function.

Copy link
Contributor

@JonasKruckenberg JonasKruckenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're goal here is to improve the DX especially when using TS, might I suggest doing a simple overload of the listen instead? All these separate handlers add a lot of code while not really feeling all that iditomatic

  async listen(event: 'tauri://scale-change', handler: EventCallback<ScaleFactorChanged>): Promise<UnlistenFn>
  async listen(event: 'tauri://menu', handler: EventCallback<string>): Promise<UnlistenFn>
  async listen(event: 'tauri://file-drop', handler: EventCallback<{ type: 'drop', paths: string[] }>): Promise<UnlistenFn>
  async listen(event: 'tauri://file-drop-hover', handler: EventCallback<{ type: 'hover', paths: string[] }>): Promise<UnlistenFn>
  async listen(event: 'tauri://file-drop-cancelled', handler: EventCallback<{ type: 'cancel' }>): Promise<UnlistenFn>
  async listen(event: 'tauri://theme-changed', handler: EventCallback<Theme>): Promise<UnlistenFn>
  async listen(event: 'tauri://close-requested', handler: EventCallback<CloseRequestedEvent>): Promise<UnlistenFn>
  async listen<T>(event: EventName, handler: EventCallback<T>): Promise<UnlistenFn> {
     // actual implementation
  }

@@ -29,6 +29,14 @@ interface UpdateResult {
shouldUpdate: boolean
}

async function onUpdaterEvent(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would be one object updater which has methods and is an event emitter, so people can use the proper .addEventListener('<updater-event>', () => {}) JavaScript idiomatic style. But for the same of not breaking this this is pretty good 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add that to the v2 todo list 😂

@lucasfernog
Copy link
Member Author

It adds a lot of code on our end, but it reduces the boilerplate on the devs code. Also it'll look better in the documentation page and it'll be easier to search (I think).

@JonasKruckenberg
Copy link
Contributor

It adds a lot of code on our end, but it reduces the boilerplate on the devs code. Also it'll look better in the documentation page and it'll be easier to search (I think).

Hmmmm yeah I see that 🤔
alright fine by me 👍🏻

* @param handler
* @returns A promise resolving to a function to unlisten to the event.
*/
async onFileDropEvent(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wdyt about making FileDropEventa proper DragEvent instead? https://developer.mozilla.org/en-US/docs/Web/API/DragEvent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can file a feature request for that.

* @returns A promise resolving to a function to unlisten to the event.
*/
async onCloseRequested(
handler: (event: CloseRequestedEvent) => void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
handler: (event: CloseRequestedEvent) => void
handler: EventCallback<CloseRequestedEvent>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not the same, EventCallback<CloseRequestedEvent> translates to (event: Event<CloseRequestedEvent>) => void and we're wrapping the event fields in the CloseRequestedEvent directly (just to add the preventDefault method).

* @param handler
* @returns A promise resolving to a function to unlisten to the event.
*/
async onFocusChanged(handler: EventCallback<boolean>): Promise<UnlistenFn> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, shouldn't we be reusing DOM concepts as much as possible? I.e. make this FocusEvent https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent with relatedTarget set to null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can file a feature request for that.

@JonasKruckenberg
Copy link
Contributor

Only have one real change suggestion and two ideas for improvements/aliging with web standards that are not blocking

@FabianLars
Copy link
Member

From an API perspective the events are tied to the appWindow though, so they'll have to be a little smart with the bundler config to only import appWindow for tauri builds - or inject dummy values.

Yeah just a really minor "drawback" (wouldn't even call it that), but again, nothing to worry/think about 🤷

Why does it make it "less obvious"? You're still just listening to an event - and it still returns the unlisten function.

I know how stupid it sounds, but "less obvious" because it moves further away from addEventListener.
That said we should probably add a note about it somewhere (faq?) either way, because this comes up often enough already with listen.

@lucasfernog
Copy link
Member Author

I think we should add a note for it on every listen function lol

@lucasfernog
Copy link
Member Author

@FabianLars better with the docs change now?

@FabianLars
Copy link
Member

Yep, i like it 👍

@lucasfernog lucasfernog merged commit b02fc90 into dev Jul 5, 2022
@lucasfernog lucasfernog deleted the feat/listen-abstractions branch July 5, 2022 19:57
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.

None yet

3 participants