ngld / OverlayPlugin

Yet another OverlayPlugin fork.
Other
229 stars 39 forks source link

A port for plugin's common.js API #148

Open dsrkafuu opened 4 years ago

dsrkafuu commented 4 years ago

Hello, I recently ported the API provided by common.js to an ES6+ class module and published it to npm registry, so it can be imported from a modern workflow with package bundler easier, with more convenient updating and maintainability. I also added full doc comment to it for future development.

I made some modification to make it possible adding or removing listeners whenever you want without calling startOverlayEvents(). I've tested this well on my own private React.js overlay but I don't know whether I should send messages to the plugin like this.

Ported API and modified functions:

npm registry:

Finally, could I ask if you would like to be added as a collaborator to this repository? That would be great help for future development.

ngld commented 4 years ago

Hm... this is kind of goes against what I intended when I implemented common.js. The dev docs explicitly mention that you're not supposed to bundle common.min.js and instead load the one I host. Most importantly I consider stuff like window.__OverlayCallback an implementation detail that could change in the next OverlayPlugin release.

The idea was that if I had to change that API, I could just add a check to common.min.js and use the old API if the script is loaded in an old OverlayPlugin version and the new API in the opposite case. That's still possible if people bundle common.js with their overlay but that would also means that each overlay would have to update before I could release the next OverlayPlugin version.

Finally, I added startOverlayEvents() intentionally to avoid common issues with cached events. A few events are triggered immediately after you register a listener for them with OverlayPlugin. This includes events like ChangeZone and ChangePrimaryPlayer. The idea behind this is that when your overlay appears, the player might already exist and might already be in a zone. You probably don't want to wait until the player (or the zone) changes so instead OverlayPlugin immediately triggers the event with the current value. However, this means that only the first listener that is registered will receive this data which can be confusing if it isn't explained anywhere.

If you register all event listeners before you call startOverlayEvents(), this is never a problem since all listeners are registered before a single event is processed. In practice, I've never had a reason to register more listeners once the page has been loaded. Just calling startOverlayEvents() in my load event listern worked well enough and you don't have to pass any events since it can detect the necessary events through the listener registration.

TL;DR: This is a nice effort but I'm not entirely sure how useful it's going to be since bundling the file might cause your overlay to break in future OverlayPlugin versions.

Sorry for the long answer. I think I tried to explain too hard why things are the way they are. ^^"

dsrkafuu commented 4 years ago

Thank you for your long answer 😁 I've been working on our own overlay with a couple of my raid group members recently, so this bundle is intended to make our own development easier. New features like simplifying data and detecting job types have also been added. Anyways, this is just a personal project and I'll try to make it work properly in the future. Your explanation of the event listeners and the immediately triggered event is very clear; it will help us in future work.