T/248: Made UI destruction a repeatable process

[oleq]

Suggested merge commit message (convention)

Other: Made the UI destruction a fail–safe, repeatable process. Closes ckeditor/ckeditor5#5372.

[oleq]

I wonder if this helps with ckeditor/ckeditor5-core#87. @pomek?

[Reinmar]

Why did you add the logging? What's the purpose of it?

The issue would be that someone would try to use a component which was already destroyed. Not that someone attempted to destroy it again.

Let's go easy here. Besides simple KISS rule, we need to be consistent here. If we'd introduce warning here, we should do that in all other destroyable classes.

[Reinmar]

In other words, let's not make our lives miserable for no good reason.

[Reinmar]

All editor tests pass without any problems with these changes.

[oleq]

Why did you add the logging? What's the purpose of it?

I found many bugs this way and I decided it's a good thing to keep it. We wouldn't be here in the first place if not for the problems with the double destruction, right?

Even if it's now possible to call successive destroy() on the same View, I still consider it a possible issue in the controller. In other words: it works, but if you do this, your code is probably wrong and will blow up anyway this way or another. Just as you shouldn't call init() twice (we even have an error for this), you shouldn't destroy() stuff many times either. It's a debugging hell lurking in the code, waiting to unleash its true power when you build something more complex on top of it.

And no, we won't have unit tests to check this thing because you'd need to remember about this case in dozens of controllers in the years to come. If that was so easy, we wouldn't be here. We failed to discover the problem for months and I'm afraid it's not the last time we're facing it.

[Reinmar]

I understand your way of thinking and if that was a cheap approach, I'd vote for it. But it isn't. We must be smart when adding excessive logging because it always means more test, more code to maintain, bigger code size, and so on.

I was actually complaining in the past about your choices what to validate because you tended to validate things like types of params. Those kinds of issues, with wrong param types in such places like API entry points are very easy to track. We should add warnings in places which are prone to hardly trackable runtime state. Inside complex algorithms, inside complex flows and whenever some important assumption is made which result might come out in a completely different place. Not on the entrance, when things are still easy to follow. Not in a place where 3 lines below an error would be thrown anyway.

And double destroy is such a thing – it's easy to follow if it causes any bug. If any day it would cause us a problem, you wouldn't need a day to find it – it'd be rather obvious. Especially that we're talking about destruction which would need to either throw... or I don't know what cause there's no state after destroy.

KISS.

I found many bugs this way and I decided it's a good thing to keep it.

So we do with tests. But we don't bundle tests into the code ;)

[oleq]

because it always means more test

Like a single test that verifies if it actually warns?

more code to maintain

Like a single line and a bunch lines of comment?

bigger code size

And yet we use it in situations where things obviously blow up too. It's hard to XMLHttpRequest.open() without an url, right?

I was actually complaining in the past about your choices what to validate because you tended to validate things like types of params. Those kinds of issues, with wrong param types in such places like API entry points are very easy to track.

I agree. But it doesn't make a point in this discussion and let's focus on the present issue. I just find that we have tools (log.warn) which may improve DX and yet we hardly use it. Just a couple of warns across thousands LOC. My point is that...

And double destroy is such a thing – it's easy to follow if it causes any bug.

...it's not only about us. We should support developers who might not be familiar with some concepts and more often than not they develop things blindly. We've been on SO for years now and people struggle even with easy, documented tasks. Helping them out with warn here and there is a very low–cost solution IMO. Unlike errors, which must be caught, you can add and remove warnings whenever you think someone may need them.