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

[WIP] Introduce the ContextWatchdog #33

Merged
merged 102 commits into from
Feb 10, 2020
Merged

[WIP] Introduce the ContextWatchdog #33

merged 102 commits into from
Feb 10, 2020

Conversation

ma2ciek
Copy link
Contributor

@ma2ciek ma2ciek commented Jan 29, 2020

Suggested merge commit message (convention)

Feature: Introduce the ContextWatchdog which will be a new watchdog for the Context feature and the main watchdog for multi-editor scenarios sharing the Context instance. Closes ckeditor/ckeditor5#6079. Closes ckeditor/ckeditor5#6042. Closes ckeditor/ckeditor5#4696.


TODO:

  • Should the Watchdog be renamed to EditorWatchdog? This would be a BC (But we'll have other BC anyway in this release in this package).
  • Make sure that the instance of the base abstract Watchdog class won't be created by mistake.
  • State management in the ContextWatchdog needs more tests
  • How the ContextWatchdog should treat the Watchdog's configuration (minimumNonErrorTimePeriod, crashNumberLimit) in case of many internal watchdogs and the main one (IMO that the crash number limit configuration should be kept for all instances together).
  • Docs
  • Should the remove accept array of strings and string?

Additional information

BREAKING CHANGE: The Watchdog class was renamed to the EditorWatchdog class and is available in the src/editorwatchdog.js file.
BREAKING CHANGE: The EditorWatchdog.for() method was removed in favor of the constructor.
BREAKING CHANGE: The EditorWatchdog#constructor() API changed, now the EditorWatchdog accepts the editor class as the first argument and the watchdog configuration as the second argument.
The EditorWatchdog editor creator now defaults to (sourceElementOrData, config ) => Editor.create( sourceElementOrData, config )

Side note: I duplicated the getSubNodes() and areConnectedThroughProperties() functions and copied and modified them in the Watchdog repository having ckeditor/ckeditor5#4699 in mind.

@ma2ciek ma2ciek requested a review from scofalik January 29, 2020 13:20
* `crashNumberLimit` - A threshold specifying the number of editor errors (defaults to `3`). After this limit is reached and the time between last errors is shorter than `minimumNonErrorTimePeriod` the watchdog changes its state to `crashedPermanently` and it stops restarting the editor. This prevents an infinite restart loop.
* `minimumNonErrorTimePeriod` - An average amount of milliseconds between last editor errors (defaults to 5000). When the period of time between errors is lower than that and the `crashNumberLimit` is also reached the watchdog changes its state to `crashedPermanently` and it stops restarting the editor. This prevents an infinite restart loop.
<info-box>
Examples presents the "synchronous way" of the integration with the context watchdog feature, however it's not needed to wait for the promises returned by the `create()`, `add()` and `remove()` methods. There might be a need
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this info box, to be honest. I don't know what you wanted to convey here and whether it is actually important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to say that the promises are chained anyway so there's no need to wait for these promises as some integrations may require running multiple add() and remove() functions in the watchdog at the same time.

@scofalik
Copy link
Contributor

scofalik commented Feb 9, 2020

I see that in watchdog.js you have changed "editor" to "instance". But "instance" sounds ambiguous. "Instance" of what? The word sounds weird when it is "alone".

How about "watched object" or "watched item" instead?

@scofalik
Copy link
Contributor

scofalik commented Feb 9, 2020

Three thoughts.

  1. ContextWatchdog.create should return a promise that resolves with the Context instance. Similarly, ContextWatchdog.add should return a promise that resolves with context item instance.
  2. Following the above, I am not sure if I like .add() with an array, after all 🤔.
  3. Also, I think that it would be nice if you don't have to specify default creator for editors. Maybe creator could take editor class as a parameter (although I am not sure if it would be differentiable from a creator function). So, maybe editor property, if type is 'editor'? Not sure about this point, though.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Feb 10, 2020

ContextWatchdog.create should return a promise that resolves with the Context instance. Similarly, ContextWatchdog.add should return a promise that resolves with context item instance.

I don't like it. The EditorWatchdog.create() also doesn't wrap the editor in the promise, because you can't assume that the editor which is returned by this method is still the watchdog's current editor instance. So, the thing you'd do on that editor would only affect the first instance.

I see that in watchdog.js you have changed "editor" to "instance". But "instance" sounds ambiguous. "Instance" of what? The word sounds weird when it is "alone".

How about "watched object" or "watched item" instead?

I added it, not changed, so maybe there would be a chance to create watchdog items in the more generic way. But I can change it to the watchdog item. 

Also, I think that it would be nice if you don't have to specify default creator for editors. Maybe creator could take editor class as a parameter (although I am not sure if it would be differentiable from a creator function). So, maybe editor property, if type is 'editor'? Not sure about this point, though.

Then in case of the custom creator you'd have to pass two things there. That's why I'm not sure about this idea.

@scofalik scofalik merged commit 76c4938 into master Feb 10, 2020
@scofalik scofalik deleted the i/6079 branch February 10, 2020 15:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants