Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

It should be possible to use file loader with asynchronous file fetching #2839

Closed
f1ames opened this issue Dec 3, 2018 · 1 comment · Fixed by ckeditor/ckeditor5-upload#88
Labels
package:upload type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Dec 3, 2018

Extracted from https://github.com/ckeditor/ckeditor5-paste-from-office/issues/44#issuecomment-443200544:

We realised that this code:

https://github.com/ckeditor/ckeditor5-image/blob/14dca372ab3e57e8aa4a8eead5d55fffe3143d5c/src/imageupload/imageuploadediting.js#L101-L137

does not have to stop the event. It can create loader instances synchronously and set the uploadId attribute synchronously. However, it does not have file instances synchronously – that must remain asynchronous. Which means that we need to be able to create loader instances like this:

const loader = fileRepository.createLoader( new Promise( resolve => {
    resolve( file );
} ) );

loader.file; // -> null
loader.status; // -> 'uninitialized'
loader.read(); // errror!
loader.id; // -> 'idoftheloader'

// promise gets resolved by the `ImageUploadEngine` once `file` instance is returned by the native > File API or whatever else...

loader.file // -> file instance
loader.status; // -> 'idle'
loader.id; // -> 'idoftheloader'

It's important that loader.id is accessible from the first moment (synchronously) so the main logic of that listener can stay synchronous.

@f1ames
Copy link
Contributor Author

f1ames commented Dec 7, 2018

I assumed that FileLoader constructor will only accept promises as it makes it consistent and does not introduce dual logic to how things works (even if it would be quite straightforward). It also prevents from confusion because providing File vs Promise will result, for example, in ability/inability to access loader.file in a sync manner. So depending what is passed, FileLoader will behave different which makes it harder to work with it.

In the same time, I assumed that fileRepository.createLoader() method could accept both File and Promise (keeps backward compatibility and makes it easier to use if one already have File instance). If File is passed, it is wrapped into a promise and passed further to a FileLoader constructor. However, this implicates some changes in how things work.

The loader.file getter

Since FileLoader accepts promise only, the file instance cannot be accessed in a synchronous manner, because it is only available after promise is resolved (and .then is always called in async manner even for already resolved promise). Now there are few solutions:

  1. Initially file is null, and after promise is resolved it holds file instance (doesn't matter if it is a property or getter).
  2. There is loader.file getter which returns a promise (resolving to a file instance) instead of a file instance.

Both solutions seems similar, however there is one difference. When getter returns the promise, one can go with:

loader.file.then( file => ... );

So it's easy to hook up into the exact moment when file promise is resolved and retrieve file instance.
If the first option would be used, such "hooking" can be done only if Promise is passed to fileRepository.createLoader() (so the above is basically done manually).

Get/destroy loader by file

Assuming that fileRepository.createLoader() method accepts both File and Promise we can use fileRepository.getLoader( promise ) or fileRepository.getLoader( file ). So if promise was passed to fileRepository.createLoader() it can be used with getLoader() method and same for the file (fileRepository will just store a map of some kind file/promise -> loader).

However, if loader is created with promise fileRepository.createLoader( promise ) and then retrieved with a file instance fileRepository.getLoader( file ):

fileRepository.createLoader( new Promise( resolve => {
    resolve( file );
} ) );

fileRepostiory.getLoader( file );

It makes it possible to retrieve a loader this way only after loader.file.then gets resolved. However, before there is now access to file instance anyway so it makes sense.

jodator referenced this issue in ckeditor/ckeditor5-upload Jan 7, 2019
Fix: `FileLoader` now accepts `Promise` instead of a `File` instance. Closes #87.

BREAKING CHANGE: The `FileLoader.file` property was changed to a getter which returns a native `Promise` instance instead of a `File` instance. The returned promise resolves to a `File` instance.
mlewand referenced this issue in ckeditor/ckeditor5-image Mar 13, 2019
…d adapter guide. Closes ckeditor/ckeditor5#1618. [skip ci]

Now it uses promises as introduced in 12.0.0 with ckeditor/ckeditor5-upload#87.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-upload Oct 9, 2019
@mlewand mlewand added this to the iteration 22 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:upload labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:upload type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants