Promise support #2988

jacekbogdanski · 2019-03-27T10:17:35Z

What is the purpose of this pull request?

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

After some research, it looks like ES6-Promise polyfill should suit our needs. It's pretty lightweight (6.17 KB / 2.4 KB gzipped) and seems to have the same API as native Promise. It's also recommended by MDN Promise docs.

Closes #2962

engineering-this

General issues:

Promises are not loaded on built version.

It looks like polyfill.js file is not included in the build.
Used library should be listed in LICENSE.md instead of Q.

engineering-this · 2019-04-15T12:40:00Z

core/promise/promise.js

+		if ( window.Promise ) {
+			CKEDITOR.tools.promise = Promise;
+		} else {
+			var script = new CKEDITOR.dom.element( 'script' );


You can simplify it by using CKEDITOR.scriptLoader.load( src, callback );.

engineering-this · 2019-04-15T12:43:08Z

tests/core/tools/promise.js

+			if ( window.Promise ) {
+				assert.areSame( window.Promise, CKEDITOR.tools.promise, 'Native Promise should be enabled' );
+			} else {
+				assert.areSame( window.ES6Promise, CKEDITOR.tools.promise, 'Polyfill Promise should be enabled' );


False positive case:
When polyfill library is not loaded test passes, as undefined === undefined.

engineering-this · 2019-04-15T13:52:39Z

core/promise/promise.js

+	 *	} );
+	 * ```
+	 *
+	 * @param {Function} resolver


Function params can be listed as attributes, like in CKEDITOR.tools.array.forEach.

jacekbogdanski · 2019-04-16T08:18:35Z

I realized the issue here - the whole core folder is minified by the builder, so there is no way to add the polyfill conditionally in that place. Instead, I created vendor folder for such 3rd party libs. It seems to be included in a built version and additionally minified, so we can use the full version for debugging.

I still would like to keep core/promise.js file instead of inlining this code inside core/tools.js, although in fact, it's tiny now.

Please, see ckeditor/ckeditor4-docs#242 for the error code (you may wanna review this one also).

engineering-this · 2019-04-16T10:49:22Z

tests/core/tools/promise.js

+		},
+
+		// (#2962)
+		'test missing promise polyfill': function() {


This test will fail in built version as CKEDITOR.loader is absent because all core modules are bundled.

engineering-this · 2019-04-16T12:49:42Z

Please, see ckeditor/ckeditor4-docs#242 for the error code (you may wanna review this one also).

I think it should be reviewed after promise is merged.

jacekbogdanski · 2019-04-16T14:09:37Z

I'm wondering how to fix the test for built version #2988 (review) - maybe it's a hint for poorly written API?

tl;dr core/promise.js module is loaded when bootstrapping CKEditor and contains logic responsible for fetching promise polyfill (very simple, yet still requires testing). I'm trying to verify the sad path when there is no promise polyfill available at vendors/promise.js:

Options which comes to my mind:

Enable unit test only for dev version
Refactor promise loading into a separate function, something like CKEDITOR.promiseLoader.load, so it can be easily tested, although adds mostly unused API and also pollutes CKEDITOR namespace
Resign from CKEDITOR.error message - at some point editor will blow with missing CKEDITOR.tools.promise type

Honestly, I'm feeling like I did some overengineering here and there is no reason for an explicit error message. WDYT?

engineering-this · 2019-04-16T14:23:47Z

Error message is helpful. I'd ignore test under condition: !CKEDITOR.loader. It's unlikely that message will work correctly in dev and not in build. Additionally we can test it manually by deleting polyfill file.

jacekbogdanski · 2019-04-16T14:33:06Z

Additionally we can test it manually by deleting polyfill file.

That would be a bit difficult to do during a testing phase, although I agree that we maybe could disable test for built version, as it's only testing if an error message occurs, not if a promise has been correctly loaded.

engineering-this · 2019-04-16T14:38:34Z

Missing an error message doesn't really break things, so I'd just check it before merging PR.

engineering-this

LG2M 👍

f1ames

I wonder if we could move ES6Promise js file from vendor dir to adapters dir. Well, this is not strictly an adapter, but now we have adapters with one jquery file and vendor dir with one promise.js file 🤔 (Btw. we cannot rename adapters to not break backward compatibility).

When it comes to unit test, there are two test promise initialization and test missing promise polyfill. Do you think it would make sense to add e.g. one more test to assert how CKEDITOR.tools.promise behaves? It will be testing native code / polyfill so I'm not sure. OTOH, if this test will start failing it means something changed in the implementation (not sure if it's probable).

I created vendor folder for such 3rd party libs. It seems to be included in a built version and additionally minified, so we can use the full version for debugging.

Is the content of this folder also minified during build?

LICENSE.md

core/promise.js

tests/_benderjs/ckeditor/static/tools.js

tests/core/tools/promise.js

jacekbogdanski · 2019-05-17T07:55:48Z

I wonder if we could move ES6Promise js file from vendor dir to adapters dir. Well, this is not strictly an adapter, but now we have adapters with one jquery file and vendor dir with one promise.js file thinking (Btw. we cannot rename adapters to not break backward compatibility).

If you are just worried about the additional folder, don't be - there is no filesystem limit we can exceed with that 😄 To be more serious - you already wrote that it's not adapter and in the future, we may have additional vendor libs with the same problem. It will be easier to keep 2 separate namespaces for that than always close eye for "not strictly adapter" argument. Although it's not something I want to fight for, if you still would like to move all vendor libs to adapter folder, it's fine for me. But the name may be misleading for the users.

When it comes to unit test, there are two test promise initialization and test missing promise polyfill. Do you think it would make sense to add e.g. one more test to assert how CKEDITOR.tools.promise behaves? It will be testing native code / polyfill so I'm not sure. OTOH, if this test will start failing it means something changed in the implementation (not sure if it's probable).

We are not testing the native Array object nor other types. I would make no exception here and keep it simple.

OTOH, if this test will start failing it means something changed in the implementation (not sure if it's probable).

The Internet would fall with such breaking native API change 😄 I think we will be fine.

Is the content of this folder also minified during build?

It is.

f1ames · 2019-05-21T12:11:57Z

Rebased on the latest major.

f1ames

LGTM 👍

jacekbogdanski force-pushed the t/2962 branch from b877c6a to 757c981 Compare March 27, 2019 10:20

engineering-this self-requested a review April 15, 2019 11:44

engineering-this self-assigned this Apr 15, 2019

engineering-this suggested changes Apr 15, 2019

View reviewed changes

CKEditorBot unassigned engineering-this Apr 15, 2019

jacekbogdanski self-assigned this Apr 15, 2019

jacekbogdanski mentioned this pull request Apr 16, 2019

Add no-vendor-lib error code ckeditor/ckeditor4-docs#242

Merged

jacekbogdanski requested a review from engineering-this April 16, 2019 08:19

CKEditorBot unassigned jacekbogdanski Apr 16, 2019

engineering-this self-assigned this Apr 16, 2019

engineering-this suggested changes Apr 16, 2019

View reviewed changes

engineering-this removed their assignment Apr 16, 2019

jacekbogdanski self-assigned this Apr 16, 2019

jacekbogdanski requested a review from engineering-this April 16, 2019 14:45

CKEditorBot unassigned jacekbogdanski Apr 16, 2019

engineering-this self-assigned this Apr 18, 2019

engineering-this approved these changes Apr 18, 2019

View reviewed changes

CKEditorBot unassigned engineering-this Apr 18, 2019

f1ames suggested changes May 15, 2019

View reviewed changes

jacekbogdanski self-assigned this May 17, 2019

jacekbogdanski force-pushed the t/2962 branch from 78761b6 to 9ba7bcb Compare May 17, 2019 08:03

jacekbogdanski requested a review from f1ames May 17, 2019 08:03

CKEditorBot unassigned jacekbogdanski May 17, 2019

jacekbogdanski and others added 24 commits May 21, 2019 14:11

Promise support.

cfeaa7b

Updated promise to expose full type.

1d6f4ed

Replaced Q with promise.

e26090d

Updated docs.

0311918

Added promise test.

c1b7c1e

Docs refactoring.

d549da2

Changelog entry.

b235922

Added license header.

747305c

Removed leftover.

a143b17

Refactoring.

fd69829

Refactoring.

438a019

Updated test.

51652be

Updated sad path.

75e9d8d

Added test verifying missing polyfill.

b352c62

Removed leftover.

a4deb63

Updated promise with unminified version.

63ab418

Update to absolute URL.

61ae9ce

Ignored test for built version.

9c4ce85

Update license.

157321e

Docs improvement.

7b5e07b

Test update.

5924a7a

Updated gruntfile.

b5c0e76

Docs.

1d5cbae

Docs rewording.

b45cf49

f1ames force-pushed the t/2962 branch from 0a33c10 to b45cf49 Compare May 21, 2019 12:11

f1ames approved these changes May 21, 2019

View reviewed changes

f1ames merged commit 680bdd9 into major May 21, 2019

CKEditorBot deleted the t/2962 branch May 21, 2019 13:11

engineering-this removed their request for review May 30, 2019 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promise support #2988

Promise support #2988

jacekbogdanski commented Mar 27, 2019

engineering-this left a comment

engineering-this Apr 15, 2019

engineering-this Apr 15, 2019

engineering-this Apr 15, 2019

jacekbogdanski commented Apr 16, 2019

engineering-this Apr 16, 2019

engineering-this commented Apr 16, 2019

jacekbogdanski commented Apr 16, 2019

engineering-this commented Apr 16, 2019

jacekbogdanski commented Apr 16, 2019 •

edited

Loading

engineering-this commented Apr 16, 2019

engineering-this left a comment

f1ames left a comment

jacekbogdanski commented May 17, 2019

f1ames commented May 21, 2019

f1ames left a comment

Promise support #2988

Promise support #2988

Conversation

jacekbogdanski commented Mar 27, 2019

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

engineering-this left a comment

Choose a reason for hiding this comment

engineering-this Apr 15, 2019

Choose a reason for hiding this comment

engineering-this Apr 15, 2019

Choose a reason for hiding this comment

engineering-this Apr 15, 2019

Choose a reason for hiding this comment

jacekbogdanski commented Apr 16, 2019

engineering-this Apr 16, 2019

Choose a reason for hiding this comment

engineering-this commented Apr 16, 2019

jacekbogdanski commented Apr 16, 2019

engineering-this commented Apr 16, 2019

jacekbogdanski commented Apr 16, 2019 • edited Loading

engineering-this commented Apr 16, 2019

engineering-this left a comment

Choose a reason for hiding this comment

f1ames left a comment

Choose a reason for hiding this comment

jacekbogdanski commented May 17, 2019

f1ames commented May 21, 2019

f1ames left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Apr 16, 2019 •

edited

Loading