Use delayIfDetached as default value in editor config #128

sculpt0r · 2021-12-20T06:06:55Z

Bump vue testing utility. It is no longer in the beta version. The newest version contains options for mount testing method which is useful in this case: https://vue-test-utils.vuejs.org/api/options.html#attachto

Adjust the component to use the new default config value. It requires moving editor inner instance initialization to the instanceReady event. Since with the delayIfDetach config options, it is possible that core replace or inline methods return null instead of editor instance.

Split mounted component logic onto separate methods - since preparing config for the editor is more complicated now.

Add safari launcher since BS Safari often ends with timeout errors. So it is possible to use safari in karma config on your local machine. However, I didn't modify the config.

As for the tests workflow. Hopefully, I was able to separate tests and test cases from each other. I have to make an ugly hack for the test with an empty editor config since I can't set there a fake observableParent. MutationObserver is disconnected while the editor switch mode, so I used that. But because of those problems, I made a follow up:

Currently, we are destroying the wrapper for the CKE4, however, we may destroy the editor first, and then clean up the wrapper. Currently, it makes no difference, but I've made a follow up: ckeditor/ckeditor4#5004

Extracted two functions in tests to util file.

Also, test changes may be a good introduction to #6 However - I feel like we need to put more effort to test refactoring.

Closes #102 , #86

github-actions · 2022-01-04T09:15:21Z

It's been a while since we last heard from you. We are marking this pull request as stale due to inactivity. Please provide the requested feedback or the pull request will be closed after next 7 days.

Comandeer

The proposed solution looks and works well. I've just only some minor comments.

We could also add a sample with a detachable component to show users how it could be used.

I've also seen that a safari launcher is added but it's not used at all. The karma config is not modified 🤔. Shouldn't it be added in place of BS Safari? Or at least added to the list of local browsers? However, it'd probably require making browsers list os-dependent.

Comandeer · 2022-01-10T10:49:16Z

src/ckeditor.js

+			}
+
+			config.on.instanceReady = evt => {
+				this.instance = evt.editor;


At the moment we have two flows of setting this.instance – here and earlier, in line 63. I don't see a need to have two different flows and I'd remove the synchronous one in favor of this async one.

Yeah... that was only for the purpose of fulfilling the tests if I good remember. So the instance in 63 is eventually set to null and it is no longer undefined. I'll correct it.

Comandeer · 2022-01-10T10:50:01Z

src/ckeditor.js

+			config.on.instanceReady = evt => {
+				this.instance = evt.editor;
+
+				this.$nextTick().then( () => {


Why is nextTick needed?

Also, we could make the whole listener async and use await here – it'd eliminate one level of indentation.

This particullar nextTick comes out of fix for IE: #120

I'll give it a few more tries later - however making this listener async - breaks a lot of tests ;)

Comandeer · 2022-01-10T10:58:05Z

src/ckeditor.js

+			let userInstanceReadyCallback;
+			if ( config.on && config.on.instanceReady ) {
+				userInstanceReadyCallback = config.on.instanceReady;
+			}


I don't know which version of JS we use, buuuut…

Suggested change

let userInstanceReadyCallback;

if ( config.on && config.on.instanceReady ) {

userInstanceReadyCallback = config.on.instanceReady;

}

const userInstanceReadyCallback = config?.on?.instanceReady;

Also, it probably can be simplified to just

Suggested change

let userInstanceReadyCallback;

if ( config.on && config.on.instanceReady ) {

userInstanceReadyCallback = config.on.instanceReady;

}

const userInstanceReadyCallback = config.on.instanceReady;

as we set the value of config.on at the beginning of prepareConfig().

We can safely skip ifs here as we check the value of userInstanceReadyCallback right before invoking it.

Unfortunately, IE11 may not allow us to use conditional chaining.

However, indeed, we already have a defensive check on config.on so this if may be skipped

Comandeer · 2022-01-10T11:05:46Z

tests/component.js

@@ -413,3 +412,85 @@ describe( 'CKEditor Component', () => {
 		} );
 	}
 } );
+
+describe( 'comp on detach elem', () => {


Suggested change

describe( 'comp on detach elem', () => {

describe( 'component on detached element', () => {

I'd rather avoid using such shortcuts. It took me a while to get what "comp" means (at first I thought it was a shortcut of "compatibility").

My bad :/ Actually, I also don't like it.

Comandeer · 2022-01-10T11:19:24Z

tests/component.js

@@ -413,3 +412,85 @@ describe( 'CKEditor Component', () => {
 		} );
 	}
 } );
+
+describe( 'comp on detach elem', () => {
+	let wrapperr;


Suggested change

let wrapperr;

let wrapper;

Comandeer · 2022-01-10T11:21:00Z

tests/component.js

+} );
+
+function createComponent( props, mountTarget = document.body ) {
+	const fakeParent = window.document.createElement( 'span' );


Suggested change

const fakeParent = window.document.createElement( 'span' );

const fakeParent = document.createElement( 'span' );

In all other places, we use document alone. I guess it's some leftover from the previous version of tests?

Must be... I don't remember to make it on some specific purpose 🤔

Comandeer · 2022-01-10T11:26:57Z

tests/integration.js

 		return Promise.all( [
 			createComponent( {}, spy ),
 			createComponent( {}, spy ),
 			createComponent( {}, spy )
 		] ).then( () => {
-			expect( spy.calledOnce ).to.equal( true );
+			expect( spy.callCount ).to.equal( 1 );


As we use sinon + chai we could write nicer assertions using sinon-chai.

Could you tell what will be nicer from your perspective? :> This looks good for me. It's short, and tells you exactly the difference between calls count.

I'm ok with both versions, but the best one for me is:

expect( spy ).to.have.been.calledOnce;

😉

If you ok with both versions - I would keep callCount. Not sure if I mentioned that somewhere, but it is a huge difference (eg. in source of the bug) if the callback wasn't called at all or was called to many times. Current assertion gives us an exact number of calls in case of fail.

Comandeer · 2022-01-10T11:31:51Z

tests/utils.js

+	} );
+}
+
+export function delay( time, func = () => {} ) {


Wouldn't it be more elegant to have something like:

await delay( 1000 );

The callback would be invoked just after the delay.

It will :) However, this will require having test functions be async. And if I applied it - there is a problem to run tests at all. Not sure why 🤔 It is possible due to the docs: https://mochajs.org/#using-async-await, but I've got:

It seems to be a bug from Babel and we need an additional plugin if we want to use async/await syntax (which can, however, result in a slightly bigger bundle).

According to common tips in vuejs/vue-test-utils@v1.0.0-beta.29...v1.0.0-beta.30

Replaced with new attachTo, which could be used in detaching tests

sculpt0r · 2022-01-10T11:44:58Z

Rebased on the newest master.

sculpt0r · 2022-01-10T11:48:29Z

I've also seen that a safari launcher is added but it's not used at all. The karma config is not modified 🤔. Shouldn't it be added in place of BS Safari? Or at least added to the list of local browsers? However, it'd probably require making browsers list os-dependent.

I didn't touch the karma config. As you mentioned that will require additional changes around it. I thought it may be a nice option for the developer to just change the config without searching and installing the launcher. Do you want me to remove it from dependencies since it is not explicitly used in the code - @Comandeer ?

Comandeer · 2022-01-10T11:57:34Z

Do you want me to remove it from dependencies since it is not explicitly used in the code - @Comandeer ?

Yes, it could be extracted as a separate ticket.

sculpt0r · 2022-01-11T08:49:27Z

@Comandeer - ready for another review.

corrected almost everything you mentioned
didn't touch spy assertion: Use delayIfDetached as default value in editor config #128 (comment)
didn't touch async/await in tests - since this PR is complex enough and introduce a lot of changes in tests

Comandeer

LGTM!

sculpt0r mentioned this pull request Dec 23, 2021

Cleanup MutationObserver ckeditor/ckeditor4#5004

Closed

sculpt0r self-assigned this Dec 23, 2021

sculpt0r marked this pull request as ready for review December 23, 2021 13:12

sculpt0r removed their assignment Dec 27, 2021

sculpt0r changed the title ~~WIP: Use delayIfDetached as default value in editor config~~ Use delayIfDetached as default value in editor config Dec 27, 2021

github-actions bot added the stale The issue / PR will be closed within the next 7 days of inactivity. label Jan 4, 2022

jacekbogdanski self-requested a review January 4, 2022 09:33

jacekbogdanski removed the stale The issue / PR will be closed within the next 7 days of inactivity. label Jan 4, 2022

jacekbogdanski removed their request for review January 10, 2022 07:32

Comandeer self-requested a review January 10, 2022 09:00

Comandeer self-assigned this Jan 10, 2022

Comandeer requested changes Jan 10, 2022

View reviewed changes

CKEditorBot unassigned Comandeer Jan 10, 2022

sculpt0r self-assigned this Jan 10, 2022

sculpt0r added 15 commits January 10, 2022 12:44

Bump vtu to beta30 & fix test with props

a31969f

According to common tips in vuejs/vue-test-utils@v1.0.0-beta.29...v1.0.0-beta.30

Bump vtu to beta31 without issues

39c8aa9

Bump vtu to beta32 without issues

a239ccc

Bump vtu to beta33 without issues

088d5f3

Bump vtu to 1.0.4 (stable) without issues

af64266

Get rid of depracated attachToDocument

4b4f9f9

Replaced with new attachTo, which could be used in detaching tests

attempt to create test on detach elem

39f2a3c

Append missing wraper destroy

7a2ae52

Add test for delayIfDetached_callback

24878b7

Clean up test code

dfd52e8

Clean up and separate namespaces in tests

ed4603e

Handle config.on.instanceReady from User

0d16c91

Add safari launcher

17a48cc

Moved delay to utils, fix failing non config test

8b078ad

Adjust test case awaiting

0ec6f82

sculpt0r added 3 commits January 10, 2022 12:44

Replace delete with delete function

ce68b29

Deeply refactor editor config initialization

b806cbb

Fix not triggered watcher after setProp

4f27151

sculpt0r force-pushed the t/102 branch from 19f73f2 to 4f27151 Compare January 10, 2022 11:44

sculpt0r and others added 8 commits January 10, 2022 13:46

Add delayed creation sample skeleton

fb48098

Add sample for delayed editor creation

9472dfc

Removed karma-safari-launcher

38e0801

Get rid of double instance assigment

8c527d4

Get rid of defensive if

cb89c42

Correct naming in tests

a537622

Get rid of unnecessary window reference

223d453

Add license information to tests utils

e2ac9e5

sculpt0r requested a review from Comandeer January 11, 2022 08:49

CKEditorBot unassigned sculpt0r Jan 11, 2022

Comandeer self-assigned this Jan 11, 2022

Add changelog entry.

18997b4

Comandeer approved these changes Jan 11, 2022

View reviewed changes

CKEditorBot unassigned Comandeer Jan 11, 2022

Comandeer merged commit eb77e1a into master Jan 11, 2022

Comandeer deleted the t/102 branch January 11, 2022 18:04

CKEditorBot mentioned this pull request Jan 11, 2022

Use delayIfDetached config for editor set to true by default #102

Closed

sculpt0r mentioned this pull request Jan 12, 2022

Update vue-test-utils usage #86

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use delayIfDetached as default value in editor config #128

Use delayIfDetached as default value in editor config #128

sculpt0r commented Dec 20, 2021 •

edited

Loading

github-actions bot commented Jan 4, 2022

Comandeer left a comment

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022

sculpt0r Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022 •

edited

Loading

Comandeer Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r Jan 10, 2022

Comandeer Jan 10, 2022

sculpt0r commented Jan 10, 2022

sculpt0r commented Jan 10, 2022 •

edited

Loading

Comandeer commented Jan 10, 2022

sculpt0r commented Jan 11, 2022

Comandeer left a comment

	describe( 'comp on detach elem', () => {
	describe( 'component on detached element', () => {

	const fakeParent = window.document.createElement( 'span' );
	const fakeParent = document.createElement( 'span' );

Use delayIfDetached as default value in editor config #128

Use delayIfDetached as default value in editor config #128

Conversation

sculpt0r commented Dec 20, 2021 • edited Loading

github-actions bot commented Jan 4, 2022

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sculpt0r Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sculpt0r commented Jan 10, 2022

sculpt0r commented Jan 10, 2022 • edited Loading

Comandeer commented Jan 10, 2022

sculpt0r commented Jan 11, 2022

Comandeer left a comment

Choose a reason for hiding this comment

sculpt0r commented Dec 20, 2021 •

edited

Loading

sculpt0r Jan 10, 2022 •

edited

Loading

sculpt0r commented Jan 10, 2022 •

edited

Loading