Asynchronous file bootstrap hooks, aka `async initFileFn` by drzraf · Pull Request #329 · flowjs/flow.js

drzraf · 2021-01-13T00:24:19Z

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*() (but opts.initFileFn is still supported)

AidasK · 2021-01-13T10:37:34Z

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

AidasK · 2021-01-13T10:41:09Z

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 · 2021-01-13T10:41:23Z

@evilaliv3 Can you review this PR too?

evilaliv3

I've no particular comments but i just looked at it briefly.

On the name maybe we could call them "Processing hooks" ?

evilaliv3 · 2021-01-13T12:07:04Z

+   */
+  *filterFileList(fileList, event) {
+    // ie10+
+    var ie10plus = window.navigator.msPointerEnabled;


I suspect this could be completely dropped and simplified. Promises are not supported on IE, even on IE11.

So actually all the specific code related to IE10 could be removed.

I think it's related for every IE/Edge browser, can't test it though

All the code is transpiled/polyfilled during compilation phase using core-js.
There is no "supported version" browserlist file but the best validation of the code would be to actually test older browsers which will be possible once the Sauce (or TestingBot) CI is up again (Related to #327 or, if not possible, #325)

This one is hard to test, you have to drop an empty folder imo and an empty file to reproduce it. I think it's easier to just leave this line. I can be always updated by IE users later

drzraf · 2021-01-13T14:25:31Z

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

With #330, it does.

drzraf · 2021-01-13T14:47:36Z

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 · 2021-01-14T08:21:56Z

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 · 2021-01-14T14:17:36Z

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 · 2021-01-14T18:18:18Z

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 · 2021-01-14T21:35:07Z

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 (#327 (comment)) so we could really compare polyfilled version with testsuite results.

AidasK · 2021-05-31T11:21:24Z

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

drzraf · 2021-06-07T15:17:01Z

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 · 2021-06-23T07:09:14Z

+- Sample use `flow.on('file-removed', ({detail: [file]}) => { ... });`
+- In an attempt of backward compatibility, some support of camelCase events exist:
+```
+flow.on('filesAdded', async (files, event) => { // v2 events prototype


maybe we should drop this backward compatibility as there is no need to update this package if it already works?

I've a project which work with current v3. I'd like to keep it using with either this version with this v3-async patch. Instead of duplicating the callback, I can just rely on the old name and that very convenient.

I opened flowjs/ngx-flow#70 about ngx-flow.
(I use to rely upon ngx-flow filesSubmitted which is part of these events changed without backward compatibility)

AidasK · 2021-06-23T07:09:45Z

  off(event, callback, options) {
    if (this.isEvent(event) || !event) {
-      console.log(`[event] Remove event listeners...`);
+      // console.log(`[event] Remove event listeners...`);


just delete it

I'm sorry we get back with topic like #312
These debug statements have been tremendously useful for me and I expect that working on this experimental code, these debug statements will keep being useful for some more time.
I'd prefer not have to maintain a debug.patch and I see no other obvious mean of instrumentation.
(That also why I was so fund of #312)

AidasK · 2021-06-23T07:10:40Z

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

drzraf · 2021-06-23T13:30:00Z

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 · 2021-06-24T15:00:56Z

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

… helper

…anually now

…ct fix some failures under particular tests sequences

…e(s) functions

…events

This is a follow-up (and proper implementation) of flowjs#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 function return promises of one (or multiple) FlowFile(s). To implement this: - 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 common to async and non-async addFiles() functions. The possibly async' nature of initializing file hit back events firing. (flowjs#319) in general and `fileAdded` in particular (flowjs#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*()` (but `opts.initFileFn` is still supported)

Added an Eventizer where all the necessary code exist. Hooks alter code processing and could be `synchronous` or not. Events are CustomEvent dispatched asynchronously for informative purpose.

…them to return

drzraf · 2021-08-06T17:27:03Z

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

AidasK · 2021-08-08T18:45:31Z

sure, merge it

drzraf changed the title ~~Async add files~~ Asynchronous file bootstrap hooks, aka async initFileFn Jan 13, 2021

drzraf requested a review from AidasK January 13, 2021 00:27

This was referenced Jan 13, 2021

generateUniqueIdentifier - is not thenable/promise which is aweful #310

Open

how do i check the file before upload? #271

Open

AidasK reviewed Jan 13, 2021

View reviewed changes

Comment thread README.md

evilaliv3 reviewed Jan 13, 2021

View reviewed changes

drzraf force-pushed the asyncAddFiles branch from f7f0422 to e120420 Compare January 13, 2021 18:14

drzraf force-pushed the asyncAddFiles branch from e120420 to 19a121b Compare January 14, 2021 15:35

drzraf force-pushed the asyncAddFiles branch 2 times, most recently from a9a6b82 to 4dc1a3a Compare January 21, 2021 14:44

drzraf force-pushed the asyncAddFiles branch 2 times, most recently from 85ebbae to e1d4041 Compare January 27, 2021 19:10

drzraf mentioned this pull request Feb 2, 2021

Support asynchronous read-file function (aka read-from-stream) #321

Merged

AidasK reviewed May 31, 2021

View reviewed changes

Comment thread src/FlowChunk.js Outdated

drzraf requested a review from MartinNuc June 7, 2021 15:17

drzraf force-pushed the asyncAddFiles branch 2 times, most recently from e1a2383 to 31d22d6 Compare June 22, 2021 18:36

AidasK reviewed Jun 23, 2021

View reviewed changes

Raphaël Droz added 16 commits August 6, 2021 14:12

tests: switch uploadSpec to createFakeServer

6761805

tests: move some useful helpers related to final file consistency

4535e20

uploadNextChunk: use transpilation instead of relying upon the each()…

499a17b

… helper

FlowFile: don't bootstrap within the constructor. This must be done m…

5341e3e

…anually now

test: bugfix. xhr_server.restore() is not enough. Recreating the obje…

deb2933

…ct fix some failures under particular tests sequences

test: added new tests for the async initFileFn and related asynAddFil…

3f8327e

…e(s) functions

test: modified tests to take into account changes related to hooks & …

d324251

…events

misc: README/CHANGELOG

efe34cc

note that fileSuccess "chunk" argument is the *last* chunk

1c7731d

Distinguish between processing hooks and async events.

90476e3

Added an Eventizer where all the necessary code exist. Hooks alter code processing and could be `synchronous` or not. Events are CustomEvent dispatched asynchronously for informative purpose.

warn if a previous event is used

caee6da

Also trigger async hooks in sync-mode (even though we don't wait for …

c50fc23

…them to return

better event name deprecation message

4601f04

Remove console.log and update README.md

3ed04c1

builds

970f276

drzraf force-pushed the asyncAddFiles branch from 31d22d6 to 970f276 Compare August 6, 2021 17:12

drzraf merged commit c8dee19 into flowjs:v3 Aug 9, 2021

drzraf mentioned this pull request Sep 9, 2021

assignBrowse / onDrop: Consider the possibility of an async initFileFn #350

Merged

AidasK mentioned this pull request Sep 14, 2021

custom generateUniqueIdentifier support async #352

Closed

Conversation

drzraf commented Jan 13, 2021

Uh oh!

Uh oh!

AidasK commented Jan 13, 2021

Uh oh!

AidasK commented Jan 13, 2021

Uh oh!

AidasK commented Jan 13, 2021

Uh oh!

evilaliv3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drzraf commented Jan 13, 2021

Uh oh!

drzraf commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AidasK commented Jan 14, 2021

Uh oh!

drzraf commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AidasK commented Jan 14, 2021

Uh oh!

drzraf commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AidasK commented May 31, 2021

Uh oh!

drzraf commented Jun 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drzraf Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AidasK commented Jun 23, 2021

Uh oh!

drzraf commented Jun 23, 2021

Uh oh!

AidasK commented Jun 24, 2021

Uh oh!

drzraf commented Aug 6, 2021

Uh oh!

AidasK commented Aug 8, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

drzraf commented Jan 13, 2021 •

edited

Loading

drzraf commented Jan 14, 2021 •

edited

Loading

drzraf commented Jan 14, 2021 •

edited

Loading

drzraf Sep 15, 2021 •

edited

Loading