Asynchronous file bootstrap hooks, aka `async initFileFn`

drzraf commented 3 years ago

This is a follow-up (and proper implementation) of #296 (reverted in e867d54)

This could be use to initialize a stream, a reader, fetch a remote resource, ... during the FlowFile initialization. asyncAddFile and asyncAddFiles are introduced which support this mechanism. These functions return promises of one (or multiple) FlowFile(s).

Implement:

An AsyncFlowFile class extending a FlowFile is created, overriding the bootstrap part
In order to keep code-duplication low, a filterFileList generator yield files in way reusable by both async and non-async addFiles() versions.

The possibly async' nature of initializing file hit back events firing (#319) in general and fileAddedin particular (#271) and the current confusión between hooks altering bootstraping and simple events.

fileAdded is now assumed an event. If we detect a return value, a warning is sent.
preFilterFile is introduced and allow filtering the raw file before bootstrap()
postFilterFile is introduced and allow filtering the FlowFile after a (possibly async) bootstrap()
filesAdded is preserved

About parameters:

initFileFn is only used during bootstrap
If such a helper exists, it's tightly related to addFile functions. As such, it was deemed adequate to provide it as a parameter to `addFile*()(butopts.initFileFn` is still supported)

AidasK commented 3 years ago

Can you check if nodejs example still words with this update? https://github.com/flowjs/flow.js/tree/master/samples/Node.js

AidasK commented 3 years ago

At first I was confused what hooks mean and how to use them. Though they are used the same as events, but you can return false in them. Maybe they should be called Events with hooks or Hook events?

AidasK commented 3 years ago

@evilaliv3 Can you review this PR too?

drzraf commented 3 years ago

Can you check if nodejs example still words with this update? https://github.com/flowjs/flow.js/tree/master/samples/Node.js

With https://github.com/flowjs/flow.js/pull/330, it does.

drzraf commented 3 years ago

About the general function prototypes.

The new code size is (still) greater than strictly necessary to just have an async initFileFn because for all other cases (no asyncInitFileFn or initFileFn not being async, ...) there is no gain except:

The ability to return the flowFile(s)
The possible future extension of having more async processing hook.

But (1) may be not that significant, and (2) is too prospective or overkill or simply not needed. One example I've in mind is: Use hooks into filesAdded to parse EXIF metadata (async operation) in the hope of filtering out the fileList according to the result or at least wait until parsing finished before calling upload().

A least intrusive change would have been to only introduce a asyncAddFiles([files], asyncInitFileFn) version (with both parameters being mandatory).

I wonder whether you guys would prefer to go a simpler way for now? (I can still reroll that part). It really depends upon what users may expect from (async) processing hooks. I also wonder if hooks couldn't just be replaced by a better object prototype extension model as Javascript permits. And if we go with more async hook, then how should we globally handle them.

AidasK commented 3 years ago

Async processing should handle every need of a user. So it should be flexible. Anyway, how big is this library? Is it really too much right now?

drzraf commented 3 years ago

Anyway, how big is this library? Is it really too much right now?

dist/flow.min.js evolution

 69673 Wed Jan 13 15:13:58 2021 -0300 (e120420) // this PR (24K gzipped)
 64127 Tue Jan 12 21:31:26 2021 -0300 (ba8a55a)
 65798 Thu Jan 7 21:03:30 2021 -0300 (db65c82)
 63996 Mon Jan 4 14:58:24 2021 -0300 (5933f9e)
 63996 Thu Dec 31 18:37:50 2020 -0300 (d536d30)
 63933 Thu Dec 31 16:22:04 2020 -0300 (f7e6d77)
 63933 Thu Oct 15 19:12:59 2020 -0300 (e1d527f)
 36507 Thu Oct 15 18:35:20 2020 -0300 (25709e7)
 36507 Tue Sep 15 16:56:03 2020 -0300 (9210467)
 61414 Wed Sep 16 11:19:53 2020 -0300 (86a87e7)
 60885 Wed Sep 16 14:12:13 2020 -0300 (6f6b6df)
 60753 Wed Sep 16 11:19:53 2020 -0300 (5fc8b4e)  // readFileFn with Promise-based chunk locking
 33949 Tue Sep 15 11:52:02 2020 -0300 (a0eafbc)
 33948 Fri Sep 11 23:45:26 2020 -0300 (cfcc849)
 33948 Thu Sep 3 14:04:58 2020 -0300 (98c63df) // started adding toolchain & polyfills 
 15333 Fri Jun 5 10:52:46 2020 +0300 (922c4b3)
 15273 Tue Jan 21 10:20:56 2020 +0200 (45dcba1)
 15159 Thu Jun 13 16:47:35 2019 +0300 (538524c)
 15143 Fri Oct 5 13:08:29 2018 +0300 (aadc5a7)
 14962 Fri Sep 21 13:07:58 2018 +0200 (a3cc2f5)
 14994 Tue Jun 20 12:57:41 2017 +0300 (172e3d1)
 14958 Fri Jun 9 11:37:59 2017 +0200 (d206e17)
 14858 Thu Jun 16 11:45:20 2016 +0300 (feff1e1)
 14824 Wed Jan 20 10:14:38 2016 +0200 (c375a34)
 14812 Tue Jan 19 13:53:20 2016 +0200 (f6109a6)
 14249 Sun Feb 1 17:38:42 2015 +0200 (08a9296)
 14190 Wed Dec 3 21:51:47 2014 +0200 (99d5e61)
 13830 Thu Sep 4 15:51:52 2014 +0300 (ceb1f82)
 13829 Thu Jul 17 23:41:23 2014 +0200 (d355db8)
 13784 Wed Jul 16 09:55:28 2014 +0300 (824d8ce)
 13766 Wed Jul 16 09:18:37 2014 +0300 (557e0ba)
 13416 Tue Apr 29 10:26:24 2014 +0300 (0525b25)
 13372 Sat Apr 12 17:18:14 2014 +0300 (082d5e8)
 13317 Mon Mar 24 17:37:46 2014 +0200 (197cb26)
 13237 Wed Mar 5 10:29:23 2014 +0200 (c0bdee9)
 13237 Tue Mar 4 13:13:44 2014 +0200 (39c2476)

AidasK commented 3 years ago

So it's basically 4x times bigger than it was. Though most projects are already using their own polyfills and for them library size should be the same. Can you calculate library size without polyfills? You could probably compare non-minimized version of original library with non-minimized version of this one.

drzraf commented 3 years ago

So it's basically 4x times bigger than it was. Though most projects are already using their own polyfills and for them library size should be the same. Can you calculate library size without polyfills? You could probably compare non-minimized version of original library with non-minimized version of this one.

Sorry we go off-topic on that PR, but the main problem is that I didn't configured a browserlist in order to selectively polyfill according to a given set of browsers (like last 2 versions, 98% US users, ...) so we are just using the default (see .babelrc (@babel/preset-env) which polyfills based on actual function usage).

Going forward, supported browsers would have to be configured using .browserslistrc file what could dramatically decrease (or increase) the resulting build. I planned this but wanted first to have the CI up and running again (https://github.com/flowjs/flow.js/pull/327#issuecomment-755373193) so we could really compare polyfilled version with testsuite results.

AidasK commented 3 years ago

Events in readme files are still listed in camelCase instead of "file-added"

drzraf commented 3 years ago

I'll rebase and fix the README. In the meantime, I'd love to get @MartinNuc input regarding ngx-flow possible adaptation (given the async nature of new function + the hook renaming)

AidasK commented 3 years ago

Great to see this fork going! I think it's almost ready for a merge :))

drzraf commented 3 years ago

Great to see this fork going! I think it's almost ready for a merge :))

How do you see the future of ngx-flow regarding this?

AidasK commented 3 years ago

@drzraf future of ngx-flow is simple. First let's release this and once this is stable, we can work on ngx-flow

drzraf commented 3 years ago

I'd like to get this merged before working on #346 and rather avoid any merge-conflict.

AidasK commented 3 years ago

sure, merge it

flowjs / flow.js

Asynchronous file bootstrap hooks, aka `async initFileFn` #329