Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

FileLoader now accepts Promise instead of a File instance #88

Merged
merged 9 commits into from
Jan 7, 2019
Merged

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Dec 7, 2018

Suggested merge commit message (convention)

Fix: FileLoader now accepts Promise instead of a File instance. Closes ckeditor/ckeditor5#2839.

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.


Additional information

See ckeditor/ckeditor5#2839 for technical explanations what was changed.

This change also required some updates in ckeditor5-image and ckeditor5-easy-image plugins (because of a change in how loader.file works and it's async nature):

@f1ames f1ames changed the title [WIP] FileLoader now accepts Promise instead of a File instance FileLoader now accepts Promise instead of a File instance Dec 10, 2018
@jodator jodator self-requested a review January 3, 2019 10:40
@coveralls
Copy link

coveralls commented Jan 3, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 041f22b on t/87 into 6bd50dd on master.

@ma2ciek
Copy link
Contributor

ma2ciek commented Jan 3, 2019

All new promises in API docs can have the Promise.<File> type.

@jodator
Copy link
Contributor

jodator commented Jan 3, 2019

All new promises in API docs can have the Promise.<File> type.

thanks for tip @ma2ciek

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use more of the Promise features both in tests (mocha nicely play with them) and in the code. Using Promise.resolve() will shorten the code a bit.

Also I've found some ugly stuff from the past (FileLoader constructor docs) which would be nice to clean up also.

ps.: Shouldn't we also update CKFinder Uupload adapter?

tests/filerepository.js Outdated Show resolved Hide resolved
tests/filerepository.js Outdated Show resolved Hide resolved
tests/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
src/filerepository.js Outdated Show resolved Hide resolved
@f1ames
Copy link
Contributor Author

f1ames commented Jan 4, 2019

ps.: Shouldn't we also update CKFinder upload adapter?

Looking at this line:

data.append( 'upload', this.loader.file );

I think we should 😄 Strangely, all unit tests are passing 🤔

@f1ames
Copy link
Contributor Author

f1ames commented Jan 4, 2019

I have rechecked ckeditor/ckeditor5-easy-image#22 and ckeditor/ckeditor5-image#258 branches after refactor and updated adapter-ckfinder package too - ckeditor/ckeditor5-adapter-ckfinder#13.

@f1ames f1ames requested a review from jodator January 4, 2019 16:52
@jodator jodator merged commit 62a8c69 into master Jan 7, 2019
@jodator jodator deleted the t/87 branch January 7, 2019 10:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants