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

Modules Overhaul #21

Closed
1 of 6 tasks
EbonJaeger opened this issue Aug 30, 2018 · 3 comments
Closed
1 of 6 tasks

Modules Overhaul #21

EbonJaeger opened this issue Aug 30, 2018 · 3 comments
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@EbonJaeger
Copy link
Contributor

EbonJaeger commented Aug 30, 2018

Currently LWC uses a lot of internal "events" that are triggered on various actions. In doing so, much of the work that exists in Bukkit events are re-implemented, badly. The entire thing is a big mess with unneeded complexity, making it very difficult to maintain. It would be much better if this were overhauled to use the Bukkit event system, reducing overhead and complexity, and greatly increasing maintainability.

This should not block the 2.1.0 release.

I am more than willing to tackle this myself, if so desired, and this can be turned into a meta-issue.


Sub tasks:

  • Make the LWC "events" into actual Bukkit events and move them to a new events package
  • Migrate all modules to event handlers
  • Dispatch the events via Bukkit instead of ModuleLoader
  • Clean up now-unneeded code
    • Scripting package
    • Module package
@Tsuser1 Tsuser1 added enhancement New feature or request help wanted Extra attention is needed labels Aug 30, 2018
@EbonJaeger
Copy link
Contributor Author

For public discussion, @pop4959 has stated reservations about refactoring these so soon, in that other plugins still use this system.

My thoughts are that indeed, some time should be given for other projects to prepare for and/or update to the new system. The code for this can be sat on for some time before merging it. There is no rush.

That said, how is it known that plugins use the current module system? I have not seen any discussion on this, and indeed that should happen in this issue.

@pop4959
Copy link
Contributor

pop4959 commented Sep 5, 2018

Yes, I think this is something to revisit at a later date. I have investigated the use of the module system in the plugin, and as far as I can tell it's been in place since 1 May 2011.

I don't think we really know yet what the full impact of rewriting this all would be. As mentioned, it's been over 7 years since the system was introduced to the plugin, and so there has been plenty of opportunities for plugins to integrate with it. For example, I found out recently that a popular plugin named ChestShop integrates with LWC. I don't believe that it uses the modules, but it is a good example of why we need to be wary of overhauling core parts of the plugin.

Also, until we have a very compelling reason to change it, I can't see why we can't keep the current system. It might be a bit clunky, but you have to admit it works. Cleaning up and refactoring the code is nice, but I think there are other issues that should be addressed first.

Thanks again @EbonJaeger for your eagerness to help out with the LWC development.

Also, if anyone has something else to add to this discussion, such as to start a list of plugins with LWC as a dependency, it would be greatly appreciated.

@pop4959 pop4959 added wontfix This will not be worked on and removed help wanted Extra attention is needed labels Jul 5, 2019
@pop4959
Copy link
Contributor

pop4959 commented Jul 5, 2019

I think I'll close this issue, since it's remained stagnant for about a year, and I still don't see much reason to change it at the moment. It is a bit messier, but as I mentioned last time, it seems the events are rather baked in at this point. In addition, the internal event system serves as a sort of API for external plugins. As such, I don't think it is something that can/should be changed at this point, without breaking things.

@pop4959 pop4959 closed this as completed Jul 5, 2019
@EbonJaeger EbonJaeger removed their assignment Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

3 participants