search

hfour / h4bff

H4's backend & frontend framework :cupid::wrench: https://hfour.github.io/h4bff/
MIT License
7 stars 2 forks source link

DO NOT MERGE - Introduce App lifecycle #25

Closed EmilH4 closed 5 years ago

EmilH4 commented 5 years ago

NOTE this is just an example of how the lifecycle could look like. We havent agreed that we will use this approach yet, so dont merge

Introduces the LifecycleApp, an application extension that calls certain overridable (lifecycle) methods in a defined order, before starting/testing/using the App. The main idea is to differentiate between phases for loading the plugins, setting and validating the env variables, create the configs, and init the app.

Check the example for the proposed way how to use it.

spion-h4 commented 5 years ago

If I'm understanding right, in order to use lifecycle methods, your app has to inherit from LifecycleApp instead of App?

That implies that if you want to use a plugin package you might need to change from App to LifecycleApp if the plugin needs lifecycle methods and then you need to change all the non-lifecycle plugins to conform to the new API. Why not support lifecycle by default in the main App class?

gphfour commented 5 years ago

This looks quite promising. I agree with @spion-h4, but if we go that way I think we should consider default empty implementations for the abstract API methods, to make it even simpler when you don't really care about the lifecycle.

EmilH4 commented 5 years ago

If I'm understanding right, in order to use lifecycle methods, your app has to inherit from LifecycleApp instead of App?

That implies that if you want to use a plugin package you might need to change from App to LifecycleApp if the plugin needs lifecycle methods and then you need to change all the non-lifecycle plugins to conform to the new API. Why not support lifecycle by default in the main App class?

@spion-h4 - You are completely correct. The rationale for the proposed solution is that ideally, we wouldnt have 2 different apps (App and LifecycleApp), so they would be merge into the main App. However, if we change directly App with this implementation, we will introduce breaking change, and will prompt bigger changes in our platform. @gphfour , I strongly believe that those methods labeled as "abstract" should be overridden in the great majority of cases, and we need that mechanism to force the "user" of the App to do that.

If you disagree with any of this, please shout. Another solution that I can propose is to have this code within the platform, and once all apps have migrated to the lifecycleApp, we can move it here (by extending App, not just copying the LifecycleApp, ofc)

spion commented 5 years ago

@EmilH4 see https://github.com/hfour/h4bff/issues/24#issuecomment-481335377 for an idea of a non-breaking way to do this

wh4everest commented 5 years ago

The bot is reminding me about this PR. Should we close it, or?

spion commented 5 years ago

We can close for now to avoid unactionable reminders diluting the value of having reminders; its always possible to re-open.

By the way, you can set up a "do-not-merge" personal filter for the reminders (unforuntately not possible to add a global filter)