KIALI-1597 Don't try to update state on umounted components #727
Conversation
| .catch(err => { | ||
| if (!err.isCancelable) { | ||
| console.debug(err); | ||
| } |
There was a problem hiding this comment.
Not sure, but shouldn't err be thrown in order to be propagated to any potential other catch blocks already defined on promise? Or is it not necessary?
There was a problem hiding this comment.
Not really needed, since both then and catch return a new promise (kind of wrapping the original one). So, by chaining, the original promise is unaffected and will call its catch callbacks.
There was a problem hiding this comment.
I understand the intent and the problem this is solving.
However it complexifies the code quite a bit. I wonder if we could have another solution that has less impact on promise usage... just thinking out loud: some kind of "PromiseRegistry" that is created for each component having promises. The makeCancellablePromise would register against that registry (passed as param), and on resolve/rejection it would automatically unregister. On unmounting component, we would call a method from that registry that cleans all subscribed promises.
The advantage would be that the Components wouldn't have to care anymore about storing and updating the cancellable promise.
Maybe I'm missing something, I don't know... WDYT? Would it make sense?
|
@jotak Yes, the code it's a little more complex. The "PromiseRegistry" would work fine if we only need to cancel. But, I think most complexities are really needed. First, look at how much changed the Most of the complexities are around sorting. I found some issues because the callbacks related to sorting, sometimes were called in incorrect order. It is more evident in the "Istio Config" list: you will see that if you sort by "Config", the list fails to sort correctly. All lists had a similar issue and this PR fixes that. Perhaps I should mention this in the main comment and also think a little more how to simplify the code for the sorting issue. |
|
@israel-hdez I think the sorting issue in Istio Config is not correlated to Cancellable promises, I mean it could have been fixed without it. I don't really see any other sorting problem in the other pages, except potentially when data is refreshed while old promises haven't completed yet... but this case could be managed I think with that "registry" thing. I'm just going to try something like that: export class CancelablePromises {
private promises: Map<string, CancelablePromise<any>> = new Map();
register<T>(key: string, promise: Promise<T>): Promise<T> {
const previous = this.promises.get(key);
if (previous) {
previous.cancel();
}
const cancelable = makeCancelablePromise(promise);
this.promises.set(key, cancelable);
return cancelable.promise;
}
registerAll<T>(key: string, promises: Promise<T>[]): Promise<T[]> {
return this.register(key, Promise.all(promises));
}
cancelAll() {
this.promises.forEach(promise => promise.cancel());
this.promises.clear();
}
}and see if it's alright to fix the cancellation issues. Goal is to minimize impact on business code. |
| const wrapper = shallow(<ItemDescription item={item} />); | ||
| expect(wrapper.text()).toBe(''); | ||
|
|
||
| resolver(health); | ||
| item.healthPromise.then(() => { | ||
| return new Promise(r => setImmediate(r)).then(() => { |
There was a problem hiding this comment.
I'm not familiar with this... is returning a promise a similar thing than calling done() , with a guarantee that it's going to be evaluated?
There was a problem hiding this comment.
It's similar. Read about it here: https://jestjs.io/docs/en/asynchronous#promises
I changed this because it was the only way I could clear the "promise stack" before the assertions. Otherwise expects were being evaluated before the component rendered (because of the internal cancelable promise).
|
PS: there's a couple of things I may not understand in the PR, maybe the best is if we can chat later today. |
There was a problem hiding this comment.
Talked to @israel-hdez , +1 for me
Maybe there's some improvement we could try to bring later but that's not going to be obvious :)
Callback of some async data fetches were trying to update the state of already umounted components. This was causing some errors to appear in the browser console. Currently, errors aren't causing any problems, but by stopping further processing when component umounts this may prevent future problems. This is also fixing some issues when sorting lists.
israel-hdez
force-pushed
the
kiali-1597
branch
from
23c8466 to
80e0e62
Compare
Callback of some async data fetches were trying to update the state of
already umounted components. This was causing some errors to appear in
the browser console.
Currently, errors aren't causing any problems, but by stopping further
processing when component umounts this may prevent future problems.