refactor(@embark/cmd-controller): re add cargo for file watcher

[jrainville]

I tried using RxJS, but I couldn't find a way to have a timeout like the one we have right now and as before. That timeout is useful to throttle instantaneous file changes, like using an IDE that enables to change and save multiple files at once. Without that throttle, it would start the pipeline for each file modified, but with it, it runs only once for all.

[ghost]

DeepCode Report (#5efe35)

DeepCode analyzed this pull request.
There is 1 new info report. 1 info report was fixed.

[michaelsbradleyjr]

It's tricky to use RxJS for this task because it produces "push flows". However, if IxJS were used there wouldn't be a need for a manual throttle, as that lib's batch operation will give the desired behavior: https://gist.github.com/michaelsbradleyjr/271c3dabdabf68412e254d52b3934e62

[jrainville]

I tried your gist with batch but I didn't find that it did the throttle correctly. Maybe I'd need to try again

[michaelsbradleyjr]

There could be something wrong with the gist as I coded it rather in the abstract. However, I tested that batch, etc. work as I understood them to work with some scripts that I have in another gist:

https://gist.github.com/michaelsbradleyjr/62d16f63a3ac318b42d8132fc178acda#file-index-js

There are 4 scripts in that gist (including a package.json) but the index.js one is easier to follow, I think.

$ node index.js
[ 1 ]
[ 2, 3, 4, 5, 6, 7, 8, 9 ]
[ 10, 11, 12 ]

In the case of the file watcher it seemed like we didn't actually care about the batched values, i.e. we filtered on the file type before consuming the observable with ix's from and batch, so that's why in the gist the forEach(async () => {...} don't have parameters.

[michaelsbradleyjr]

Thinking about the throttle, I can see how the leading delay could be helpful, i.e. for more reliably getting one trigger versus two when there is a relatively big time gap between batches, e.g. between multi-save operations in an IDE. I'm thinking currently about how to put it between ixFrom and ixBatch or between the observable and ixFrom.

[michaelsbradleyjr]

See: https://gist.github.com/michaelsbradleyjr/271c3dabdabf68412e254d52b3934e62#file-filewatcherevents-js-L20

In addition to the highlighted line 20, see lines 4 and 25.

By introducing ix's debounce into the pull pipe before executing forEach I think we get the desired behavior.

Thinking through it in steps...

1. forEach isn't running because there hasn't been an event.
2. event happens and so we get a batch for just that event, and let's say it takes 10 seconds for the build (probably not, but for illustration).
3. it was a multi-save so right behind the event are five others that arrive within 50ms of the first one — they get ignored because of the debounce.
4. after 2 seconds you realize you meant to change/save another file and so you do so, and then 5 seconds later another change/save — those get batched and not ignored because we're past the 50ms debounces but before the 10 second build has finished.
5. after 10 seconds the next batch is pulled, so another rebuild, but only one because of the batching, and it's a desired rebuild because the changes in step 4 weren't part of the first rebuild.

---

See: #1855 (comment).

I updated the gist to debounce the Rx pipes:

https://gist.github.com/michaelsbradleyjr/271c3dabdabf68412e254d52b3934e62#file-filewatcherevents-js-L2-L4
https://gist.github.com/michaelsbradleyjr/271c3dabdabf68412e254d52b3934e62#file-filewatcherevents-js-L20
https://gist.github.com/michaelsbradleyjr/271c3dabdabf68412e254d52b3934e62#file-filewatcherevents-js-L25

Thinking through it in steps...

1. forEach isn't running because there hasn't been an event.
2. an asset event happens so there is going to be a batch for one event, and let's say the build will take 10 seconds (probably not, but for illustration).
3. it was a multi-save so right behind the event are five other asset events that arrive within 50ms of the first one — owing to the behavior or Rx's debounce the event in step 1 and the first four events in this step are ignored; just the last event comes out of the Rx pipe and into the Ix batch and then the build starts.
4. after 2 seconds you realize you meant to change/save another file and you do so, and then 5 seconds later another change/save — those get batched and not ignored because each time we're past the 50ms debounces in the Rx pipe but before the 10 second build has finished in the Ix forEach.
5. after 10 seconds the next batch is pulled, so another rebuild, but only one because of the batching, and it's a desired rebuild because the changes in step 4 weren't part of the first rebuild.

[jrainville]

@michaelsbradleyjr thanks for looking into it. gets me to learn more.
The only downside I see of the RxJS method is that it's really less understandable for newbies (aka me). I know we want to get rid of async, but in this case, isn't the cargo just easier to understand and giving the same result?

[michaelsbradleyjr]

Thinking about it some more, in this situation it would also work just as well to use Rx's debounce; impl would be mostly the same but with the following additions/changes:

const {interval: rxInterval } = require('rxjs');
const {debounce: rxDebounce} = require('rxjs/operators');

ixBatch(ixFrom(assets$.pipe(rxDebounce(() => rxInterval(50))))

It doesn't really matter for this purpose whether the debounce is happening in the push or pull pipe, the effect will be the same.

[michaelsbradleyjr]

but in this case, isn't the cargo just easier to understand and giving the same result?

Yes, the results should be the same with async.cargo.

Thinking in terms of pipes — push and pull — is, I believe, generally more powerful and provides for better composition than the patterns in the async library. (I may be wrong about that, of course).

However, if you look at the docs site for async and then operators suites for Ix and Rx (just the names, I mean), it's clear that there is huge conceptual crossover:

https://github.com/ReactiveX/IxJS/tree/master/src/asynciterable/pipe
https://github.com/ReactiveX/rxjs/tree/master/src/internal/operators

That is Rx and Ix (those two are complementary not either/or) and the async lib ultimately help a JS programmer accomplish the same things.

Which approach is better and/or which is easier to understand? Personal preference probably plays a big role in answering that question. I found many of the async examples for the fancier functions hard to understand at first, while the Rx/Ix concepts became clear pretty quickly.

Whatever you think is best, I'm totally onboard, just always like to look at a problem from different angles.

[michaelsbradleyjr]

For what it's worth, I experimented with debounce in the Rx pipe vs. in the Ix pipe and I found that debouncing the push pipe seems to work better:

https://gist.github.com/michaelsbradleyjr/5abf097e9c2b99d101003d6c2cafb1ed

With that script I get:

$ node debouncePipe.js 
from: 3
batched: [ 3 ]
from: 5
from: 8
from: 9
batched: [ 5, 8, 9 ]
from: 12
batched: [ 12 ]

That's exactly what I expected.

Now, if line 32 is commented and 34 is uncommented, so that the pull pipe is debounced instead of the push pipe, I get:

$ node debouncePipe.js
from: 1
batched: [ 1 ]
from: 3
from: 4
from: 5
from: 6
from: 8
from: 9
batched: [ 3, 4, 5, 6, 8, 9 ]
from: 10
from: 12
batched: [ 10, 12 ]

I was surprised by the difference — I thought it would be very similar. After running it several times, I find the output is consistent. Notice how from: 5 is reported but it seems like there wouldn't have been enough time for the 50ms debounce to expire; the same goes for some of the other from:.

I suspect it has to do with the characteristics of the async pull generally in relation to the timer (maybe microtask vs. macrotask race?); or there could be a subtle bug in Ix's debounce and/or room for improvements in its implementation.

---

I updated #1855 (comment).

I also filed an issue on the Ix repo: ReactiveX/IxJS#282.

[iurimatias]

I suggest instead of relying on outputDone we specifically wait for await engine.events.request2('pipeline:generateAll'...), this way it's very explicity and we don't have to rely on an implicit event that might or might not be there

[jrainville]

Fixed