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

Restructure Broadcasting docs #44

Merged
merged 8 commits into from
Sep 16, 2024
Merged

Restructure Broadcasting docs #44

merged 8 commits into from
Sep 16, 2024

Conversation

gwleuverink
Copy link
Contributor

@gwleuverink gwleuverink commented Sep 12, 2024

Hi @simonhamp. I've set out to add the broadcasting to IPC feature #367 to the docs

Since this feature's API is the same as listening to native events I did not add a new section, I've integrated this information into the existing structure.

I did find that the story for working with event broadcasting could be a bit more clear. I've reorganised the page to provide a more logical flow and better communicate how to work with events.

I understand that you're currently working on updating the documentation, and this PR is meant as a contribution to that effort. I hope these changes can serve as a useful reference or starting point for further improvements to the Broadcasting documentation.

Please feel free to use, modify, or disregard these changes as you see fit in your ongoing documentation work. I'm happy to discuss or make further adjustments if needed 👍🏻

Copy link
Member

@simonhamp simonhamp left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! Great work 🙏🏼

I've made some changes to tighten up the copy and not present too many alternative approaches - especially where these are governed by other libraries, such as Livewire, so we limit how much we have to make sure our docs stay up to date with changes out of our control.

I've also removed references to Websockets... the more I contemplate it, I just don't think using Websockets is the right approach - it's really heavy and has lots of jagged edges and loose parts... it just feels really painful. I'd love it if we can make the IPC route a really robust and cross-platform mechanism for achieving real-time updates to the front-end.

So these docs reflect this.

@gwleuverink
Copy link
Contributor Author

Perfect 👍🏻 It's nice and concise like this.

I'm on board with dropping WebSockets - IPC gives us a solid, straightforward approach for client-side listening now. It's great to have one clear non-ambiguous method.

@gwleuverink
Copy link
Contributor Author

gwleuverink commented Sep 12, 2024

While we're on the topic, do you think having a client side api to listen to events makes sense now Echo is out of the picture?

You might consider injecting a global helper object. Maybe something like this:

@use('Native\Laravel\Events\Windows\WindowBlurred')

<script>

Native.on('{{ WindowBlurred::class }}', payload => {
    //
})

</script> 

This way we :

  1. don't have to manually filter events in a listener that fires on every server event
  2. don't have to manually add the ipcRenderer to our bundle. (batteries included when otherwise not having a bundle when starting a fresh Blade/Livewire project)
  3. mirrors the wording of the #[On('native:Foo')] in Livewire. Making the syntax somewhat symmetrical across the stack

While a small addition I think it makes the Broadcasting story feel a lot more fleshed out

@simonhamp
Copy link
Member

Yep this is a great suggestion

@gwleuverink
Copy link
Contributor Author

gwleuverink commented Sep 13, 2024

I've drafted a PR adding this. I'll sleep on it and revisit tomorrow

NativePHP/electron-plugin#38

@simonhamp
Copy link
Member

All ready to go. I'll merge this once the releases are done next week

@simonhamp simonhamp merged commit 325e1e7 into NativePHP:main Sep 16, 2024
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