UI: Adds controller lifecycle reset hook

[johncowen]

This adds a Controller.reset hook through my app, right now it's mainly used if you use the WithListeners mixin in a Controller

Also see #4789 (comment) which is partly related to this and the discussion above that comment gives some background on the inclusion/omission of a setup-like hook. The likelihood is we will add one 🔜 , just we don't think it should be called setup as it could potentially be called when you aren't 'setting up'.

This also adds an improvement to the dynamic 'psuedo-super' call, just to bind the method correctly so this remains what it should be if there is a 'super' to be called.

[gregone]

Code LGTM but why not create a separate mixin for Controller instances, like controller-listeners instead of reusing with-listeners?
I'm worried that this will cause maintainability issues and potential unwanted side effects later.

[gregone]

Also: tests?

[johncowen]

Hey Greg! 👋

Thanks for taking a look at this much appreciated! Hopefully I can allay your worries! 😄

Do you mean have a WithComponentListeners and another WithControllerListeners?

If so, potentially I could do that, but as they are doing the exact same thing (performing cleanup/teardown when something is no longer on the page) I feel its best to keep that in the one place. The only reason there is some logic here is that Ember doesn't seem to have a generic name/hook for performing an action when an object is no longer on the page.

Options:

1. Have a WithComponentListeners and another WithControllerListeners. We'd be duplicating functionality only for the reason that one object's 'do something on leave' method is called willDestroyElement and another has (a custom) reset plus willDestroy. I feel making duplicated mixins would be hard to maintain and possibly produce hard to track down bugs later. Furthermore I believe you could mix this in to plainer EmberObjects also, this would mean a third WithEmberObjectListeners?

2. We could make a Mixin or parent class (say WithListeners) that contains the functionality but didn't actually do anything and then use that mixin for 2 more mixins WithComponentListeners and another WithControllerListeners that calls the parent class or mixin depending on what method it uses for 'when I leave the page'. In my eyes this is better than option 1, as we are centralizing the actual functionality, and it's cleaner, but.. The whole mixing in a mixin to a mixin 😬 , or a long-ish inheritance chain is something I generally try to avoid even though this is probably the purist way to do it. The abstract WithListeners mixin could also be confusing as even though it looks like you could just mixin this in to 'add listeners' but it wouldn't actually do anything.

3. Dynamically set up the correct method depending on 'what I am', so make the name of the method a variable and set it depending on 'what I am'. This is the option I chose, it centralizes the functionality without too much mixing-in/extending, is pragmatic and feels more javascript-y to me 🤷‍♂️

What would your preference be there ^ ? Or can you think of any other options I could use?

I'm worried that this will cause maintainability issues and potential unwanted side effects later.

Could you explain in more detail what these issues or side-effects might be? If there is something that might affect us I'd rethink this.

Also: tests?

This is generally plumbing, not logic/functionality.

1. If this doesn't work all my acceptance tests fall apart. So it is being tested, just at a 1,000ft level.
2. There's not a lot of logic here to unit test so I probably wouldn't unit test this as a priority. If I was to unit test this I'd pretty much be testing instanceof and push. That's no absolute reason not to unit test this but I suppose it's about priorities.
3. Integration testing is in many cases 'faster acceptance testing' usually helping you to fail early. The integration testing across the Consul UI is pretty light in general, most things are covered by a good unit testing coverage and high level of acceptance testing. We generally use integration testing in the Consul UI for performance testing where we'd like to test the speed/performance of smallish systems (not isolated units) where you need the smaller system to be isolated from the entire system. Additionally, how we have ember set up negates the 'failing fast' benefits of integration testing, as the acceptance testing (slowest) is run first when using Ember testing (acceptance begins with a and integration with i, unit tests are the fastest but being a u are run last 🤷‍♂️ )
4. The listeners functionality that this 'plumbs' into Ember objects is pretty well tested ( https://github.com/hashicorp/consul/blob/ui-staging/ui-v2/tests/unit/utils/dom/create-listeners-test.js )
5. Lastly here, this is going down onto ui-staging (my integration branch) not master, I plan on PRing another larger change today which does require deeper testing and it won't all be in the same PR, but I'll be PRing more tests before I finally merge my integration branch down onto master in the New Year. This is how I did my initial Ember 1 > Ember 2 upgrade of the Consul UI and it worked quite well.

Thanks

John

[johncowen]

Hey @gregone !

Not sure if you meant to hit the approve button here or not (maybe you did? with the LGTM?), but I'll probably merge this tomorrow unless you have any major issues with me doing that.

Shout me if you want to discuss anything, I'll probably wait until past midday before I merge.

Thanks!

John

[gregone]

I have still have thoughts on the mixin location, but that can be refactored later if needed.