fix(removeWidget): check for widgets.length on next tick

[iam4x]

I'm using the removeWidget API for Angular InstantSearch when the app need to unmount a widget.

If the page is unmounting all the widgets, it will call removeWidget() for every widget present on the page and this will result of make multiple requests to Algolia even thought we are unmounting the whole page.

In order to workaround this issue, I'm deferring the execution of the condition this.widgets.length > 0 on the next tick so every widgets would have been removed.

This is an easy-fix for now until we can maybe think about a better API with InstantSearch core 👍

[algobot]

Deploy preview for algolia-instantsearch ready!

Built with commit 1d344f1

https://deploy-preview-2831--algolia-instantsearch.netlify.com

[Haroenv]

does wrapping it in Promise.resolve also fix it?

[iam4x]

Yes it should have the same behaviour 👍

[samouss]

@iam4x agree that's an easy fix, we have the same strategy in React InstantSearch. But we should not rely on that since it's not predictable at all. For example, in React when they will launch the async rendering the lib could cause multiple request because of that. We should find a proper API to handle this case, it's a recurring problem.

[Haroenv]

How can we handle both “yes we want a new request if a widget is unmourned, but none if we’re also going to delete everything”. Maybe requestIdleCallback, but that doesn’t work in node. All I can think of is that we need to have a queue that must be flushed, but I can’t think of an implementation without timers

[vvo]

The only way to do it properly in every flavour, without relying on setTimeout 0, is to know the inner details of the framework (react, angular, vue) behaviour about mounting/unmounting/rendering/life cycle. For example maybe in Angular unmounting hooks are called and scheduled using setTimeout 0, maybe with Promise.resolve() or anything..

Once you know that, you can build a small scheduler inside the framework (angular, react, vue). This scheduler must be responsible for stacking widgets mounting/unmounting to avoid calling .search() everytime.

Which also means from my POV that nor removeWidget, nor addWidget should be responsible for calling .search(). At least flavours other than the vanilla one should be able to call removeWidget() without having it triggering a search (example: removeWidget({autoSearch: false})).

The vanilla flavour could still benefit from having search called automatically that would be scheduled in the way this PR propose it (but maybe with just Promise.resolve() which is the shortest microtask scheduler).

Knowing the limitations of this fix and it's implication is also important for other frameworks. And we need to check the behaviour when used in Vue (cc @Haroenv).

tl;dr; I am not sure we can find a generic way for every flavour, at least every framework has its own way of handling asynchronous mounting/unmounting and rendering. So we need to understand them to build a sort-of generic solution where the stack scheduler can be built in core, but async semantics needs to be passed by flavours (as an option).

[bobylito]

I have pretty much the same opinion than @vvo on the problem. I'm also for integrating this change for now and iterate.

For the glory, allow me to add some elements:

First thing, I think we should define the problem. We want to batch operations and yet minimize the waiting time before the execution. It's what @vvo mentioned when he talked about scheduling. This problem is inherently based on time. That's why we cannot do without time based callback.

Maybe using some heuristics we could cut the time. For example, if we've already unmounted all the widgets, maybe it's not meaningful to wait for another x milliseconds to trigger a new search (considering that it's meaningful to do one at all), so we could shortcut the waiting time.

Beyond this definition of the problem, we can also say that in our case we have a lot widget removal and therefore lots of new requests. For 10 removed widgets, we have 10 requests. If we could batch in 2 of 5, we'll 5x less queries which is already good. In that kind of problem, better is already good.

Eventually when renderers are all asynchronous, it will mean that we'll have time when the system will be stable and unstable. I hope that the frameworks will provide an API for knowing when the system is back to being stable so that we can do the cleanup. Hence, as proposed by @vvo, eventually a method like start that triggers the search again could be useful.

But then, here I think that if this change actually helps mitigating the issue on Angular that's a good step. Eventually, as frameworks evolve we should be able to provide more control to the implementors of framework flavors.

[Haroenv]

I'm +1 on @vvo's idea of putting this under a flag

[bobylito]

I'm +1 on @vvo's idea of putting this under a flag

I don't think it's necessary.

[bobylito]

I think it's a good first step. It should not be commited on develop though. Can you put that on feat/2.7 @iam4x ?

[vvo]

I'm +1 on @vvo's idea of putting this under a flag

To be clear the thing I was proposing to pass under a flag is the decision to call .search() or not in add/remove (to be able to call search yourself when you want). But that's more for a future change than for the current fix.

I was more providing context to @samouss comment about the fact that in React this might or might not work ultimately.

If right now this is fixing a good issue, and it does not breaks anything then why not.

The only thing that could break, since you are moving from synchronously calling search to asynchronously, could be people relying on the former behaviour.

A safe bet would be to be able to put the setTimeout on Angular side maybe, like this:

// this is code on Angular InstantSearch side
removeWidget(function done() {
  if (instance.widgets.length > 0) {
    setTimeout(() => helper.search(), 0);
  }
})

Used like that, removeWidget won't call helper.search automatically

Until we have a proper solution/scheduler in IS.js.

Your call here.

[samouss]

You take a look at this package for the perf about timeout / Promise etc...

https://github.com/jamiebuilds/tickedoff#perf