Add functionality to cancel awaiting for delayed editor creation

[sculpt0r]

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches that 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

Did you follow the CKEditor 4 code style guide?

Your code should follow the guidelines from the CKEditor 4 code style guide which helps keep the entire codebase consistent.

• PR is consistent with the code style guide

What is the proposed changelog entry for this pull request?

* [#4641](https://github.com/ckeditor/ckeditor4/issues/4641): Add cancelation for delayed editor creation.

What changes did you make?

Give an overview…

A new config function was added. It works in the same manner as the already existing callback. So the User pass a callback which will be invoked with cancelation function as an argument. It could be stored and invoked whenever convenient.

I don't want to introduce a new type that might be returned by editor creation. This requires changes in (probably all) integrations.

Which issues does your PR resolve?

Closes #4641

[github-actions]

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.

[github-actions]

There was no activity regarding this pull request in the last 14 days. We're closing it for now. Still, feel free to provide us with requested feedback so that we can reopen it.

[jacekbogdanski]

I was rather imagining this feature as slightly different. Currently, we have very outdated documentation for both CKEDITOR.replace and CKEDITOR.inline functions indicating that these always return an editor. Unfortunately, it's no longer the truth since it's possible that these methods will return null if the integrator decided to use the delayed initialization feature.

Instead of returning null, we could actually return a handle allowing us to clear the editor initialization.

var clearEditorInitialization = CKEDITOR.replace( 'editor' );
clearEditorInitialization();

Since it's JavaScript and type checking is a bit annoying, we could actually return an object with status property similar to editor.status

const editor = CKEDITOR.replace( 'editor' );
if ( editor.status === 'delayed' ) {
  editor.cancelInitialization();
}

WDYT @sculpt0r @Comandeer ?

[Comandeer]

Actually, why not both? In some cases, you would like to have an interval and in some other cases, the better option would be to manually cancel the initialization.

So I'd add the method proposed by @jacekbogdanski to what @sculpt0r already proposed.

See #5133 (comment).

[jacekbogdanski]

Well, from my understanding both approaches do basically the same, but the proposed approach in this PR introduces a more convoluted way of clearing intervals:

// config option:
config.delayIfDetached_cancelInterval = function( cancel ) {
 // During every interval, you have to make a decision whether you want to execute the `cancel`
// function to prevent editor initialization. It forces you to (in most cases) keeping some separate reference
// to a condition under which editor initialization should be prevented. E.g.:
  if ( component.destroyed ) {
    cancel();
  }
};

// handle
// You just keep a reference of the specific handle that may be used to prevent editor initialization:
this.state.cancelEditor = CKEDITOR.replace( 'editor' );
// ...
onDestroy() {
  this.state.cancelEditor();
}

Since we already have so many config options and our documentation is outdated at the moment, the second method seems to be a more natural pick for me. Keeping both, such similar options look just redundant. From the performance side of view, the second one is also less resource-consuming as the callback is called only once, but it's rather micro-optimization than a real benefit.

[jacekbogdanski]

I believe that we should provide a wrapper over clearInterval method. Currently, an interval is cleared on the first code execution. This logic should be also moved inside setInterval callback, not outside.

[Comandeer]

Well, from my understanding both approaches do basically the same

You're right, @jacekbogdanski. I was mistaken by the name of the config variable (I assumed that the interval variable takes… well, interval – so the  number of seconds). But I see that it just takes a function that cancels the interval.

In such a case, I'm for what @jacekbogdanski proposed.

[sculpt0r]

Since we already have so many config options and our documentation is outdated at the moment

Actually, it was possible to get null way before the delay functionality was introduced. Eg. when the provided element didn't exist.

Actually, there was an idea to return the callback function when you want to use https://ckeditor.com/docs/ckeditor4/latest/api/CKEDITOR_config.html#cfg-delayIfDetached_callback - but we decide to config option to not break the so far behavior: only null and editor instance is returned. As for this PR - I just wanted to stick to the previous approach. What is more - changing the possible returned value also causes more work to do in integrations. However - if we are fine with this - I'll adjust this PR to @jacekbogdanski proposal :)

Just to be sure @jacekbogdanski and @Comandeer you are ok with ☝️

[jacekbogdanski]

Actually, it was possible to get null way before the delay functionality was introduced. Eg. when the provided element didn't exist.

I would expect an error throw in that situation, but it's not something we want to change right now.

Regarding this configuration option, from what I understand it won't result in an interval so we don't need to provide any additional option to clean it up?

There is one limitation to the originally proposed implementation - you can't tell which editor you are canceling. So in the case when you have multiple delayed editor instances, you will have to cancel all of them. With a handler, you can distinguish which editor gets canceled.

What is more - changing the possible returned value also causes more work to do in integrations.

Do you see any potential breaking change here? The only problem I have with returning a handle is that some existing integrations could rely on checking whether creation methods returned a null - it sounds like a poor verification since you should rather check if the delayed initialization option is enabled, but there is some small risk of breaking change.

[sculpt0r]

Regarding this configuration option, from what I understand it won't result in an interval so we don't need to provide any additional option to clean it up?

Nope, we don't have to clean it up. I mentioned it to show that provided solution has similar logic to sth we already have done around this feature.

There is one limitation to the originally proposed implementation - you can't tell which editor you are canceling. So in the case when you have multiple delayed editor instances, you will have to cancel all of them. With a handler, you can distinguish which editor gets canceled.

I think that you are able to distinguish the editor instances. Because you pass individual configuration for each replace creation. If you have more editors created at once - it might be cumbersome to distinguish them. But OTOH if you want to cancel specific editor creation - then you probably track that editor already, so you could do sth like:

var editorsCancels = [];

function trackEditorCanceling( editorId ) {
	return function cancelCallback( cancelFunction ) {
		editorsCancels[ editorId ] = cancelFunction;
	};
}

CKEDITOR.replace( editorElem, {
	delayIfDetached_cancelInterval: trackEditorCanceling( editorId )
} );

// Then when you are sure that a certain `editorId` should be canceled, you might:

editorsCancels[ editorId ]();

The above code should work after corrects you mentioned in: https://github.com/ckeditor/ckeditor4/pull/5133/files#r849471320

I would expect an error throw in that situation

I believe this error msg should be present in the console: https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#editor-incorrect-element

☝️ This reminds me, that we also have https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#editor-delayed-creation & https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#editor-delayed-creation-success - so probably in wrapper mentioned: https://github.com/ckeditor/ckeditor4/pull/5133/files#r849471320 we should also add another error code WDYT?

[sculpt0r]

Do you see any potential breaking change here? The only problem I have with returning a handle is that some existing integrations could rely on checking whether creation methods returned a null

There shouldn't be any issues with React - this problem seems to be invalid for this integration.

In Vue - I forgot that we already store the instance onInstanceReady - so the returned handler will be simply ignored. Also, all the magic happened on that event - so if the User cancels the creation - this component simply won't initialize. However - that probably needs some further clean-up? Like maybe removing the entire component? Sth to verify. But seems like no big deal.

In Angular - Looks the same as Vue - so without onInstanceReady nth initialized :)

[jacekbogdanski]

Because you pass individual configuration for each replace creation.

Yes, I forgot about that method.

I believe this error msg should be present in the console: https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#editor-incorrect-element

Alright, so it looks like null return value was actually not expected as a valid result previously, since we are getting an error log for that. But currently, it is a valid return value so that information should be reflected in docs (or updated with Function type if we return a handle).

☝️ This reminds me, that we also have https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#editor-delayed-creation & https://ckeditor.com/docs/ckeditor4/latest/guide/dev_errors.html#editor-delayed-creation-success - so probably in wrapper mentioned: https://github.com/ckeditor/ckeditor4/pull/5133/files#r849471320 we should also add another error code WDYT?

That's weird that we are using warnings like that in the first place. Originally, we were using warnings to indicate some error information that is not interrupting editor functionality, like uploading a file to an invalid endpoint. A warning means that there is something requiring integrator attention - but in the case of this feature, delayed initialization is an expected outcome. But I would avoid changing it right now, so we are not introducing another confusion.

But OTOH if you want to cancel specific editor creation - then you probably track that editor already, so you could do sth like:

I still have a feeling that this logic is overcomplicated to what we can accomplish by returning callback directly from creation methods. Storing callbacks from another callback execution looks unpleasant and introduces unnecessary confusion. An integrator has to assume that callback will be attached synchronously (so there is no risk that editorCancels from your code listening is empty after executing CKEDITOR.replace) and understand that the delayIfDetached_cancelInterval function is called every interval tick. I'm for sticking to returning a callback directly from the creation method if you don't see any potential breaking changes with such an API update.

[sculpt0r]

Rebased onto the newest master.

[sculpt0r]

@jacekbogdanski - ready for another review :)

• implemented the cancelation as a function returned from replace, inline, and appendTo. The replaceAll and inlineAll remain untouched - but they didn't return anything so far either. Assumed that they are for simple solutions. If the User requires more control over a bunch of editors - he will create his own loop and will take care of each returned value.
• corrected the documentation for return type from above methods (added null & Function). Not sure if we should expand the description of function return type or just add a link to the https://ckeditor.com/docs/ckeditor4/latest/features/delayed_creation.html where the new feature will be described?
• Current version also fixes your last comment: Add functionality to cancel awaiting for delayed editor creation #5133 (comment)
• the PR for docs will be prepared after we decide on a certain solution.
• manual test tag was corrected to 4.19.0
• one test case from tooling was corrected to expect function when the interval method is used to create editor with the delay.
• small improvement for the above tooling test was made: extracted to a single variable the assertion message that helps to identify the test case.

I thought about your proposal with returning editor as promise and from the development perspective, it will be very interesting and challenging. However, there are a lot of topics to cover with such a solution. Also, that changes the basics of CKEditor - the editor object itself. I'm not sure if we want to introduce such changes at all. This solution is a rather edge case and mostly the integrations use it as a fix for the frameworks behaviors.

So to not enlarge this PR too much - I decided to only introduce the minimal changes that allow canceling editor creation.

[jacekbogdanski]

I was sure that this PR is ready for a final review so I've helped a bit with some changes, but seems we are still lacking valid unit tests. Please, make sure to pull my commits before working on it.

Good documentation is essential for this feature as editor creation functions are basically the most often used public API in the whole editor codebase. Except for the missing explanation about this new return type, we were also missing information about the delayed initialization feature in docs since 4.17. I've added documentation for your changes.

Regarding replaceAll and inlineAll - these functions are a bit problematic, as they are not returning anything. It seems unlikely to me that someone would use the delayed initialization feature with them, so for now we can just extract a separate ticket for it and see if it gets any reactions.

Please, add the missing test case and we should be good with these changes.

[jacekbogdanski]

This test is not good enough. It only checks the return type of the creation method, but we are not testing the outcome of using that callback.

[sculpt0r]

Regarding replaceAll and inlineAll - these functions are a bit problematic, as they are not returning anything. It seems unlikely to me that someone would use the delayed initialization feature with them, so for now we can just extract a separate ticket for it and see if it gets any reactions.

Done: #5167

[jacekbogdanski]

It's not mutually exclusive. API docs should have enough information that integrators can work with that without reading a separate guide. Also, we were missing cross-reference links making the guide more discoverable.

[sculpt0r]

@jacekbogdanski - ready for another review.

• Added missed test case for cancelation editor creation. So now we are testing whenever the returned callback is working as expected. Still preserve the one mentioned: Add functionality to cancel awaiting for delayed editor creation #5133 (comment) - it seems to be good to have returned values types under control - at least a little bit.
• Corrected two misspells in tests cases (naming)
• I didn't change anything in docs - since I assumed your changes covers the missing parts?

[jacekbogdanski]

We are still lacking some changes in unit tests, although I will fix that issues in a moment.

[jacekbogdanski]

That's still not totally valid test case. We should make sure that the editor is not initialized. Checking internal calls is not enough in that regard and we should make sure that the creation method has not been executed. It would be best to not use spy at all, but editor initialization takes such long that simplifying it with spy on creatorFunction should be enough.

[jacekbogdanski]

Most tests that check against editor not being null are now false positive, as a function callback is also not null. Also, we are not ensuring that the delayed initialization feature has been cleaned up which impacts test isolation.