Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watchdog support for the Context feature #6079

Closed
ma2ciek opened this issue Jan 16, 2020 · 5 comments · Fixed by ckeditor/ckeditor5-watchdog#33
Closed

Watchdog support for the Context feature #6079

ma2ciek opened this issue Jan 16, 2020 · 5 comments · Fixed by ckeditor/ckeditor5-watchdog#33
Assignees
Labels
bc:major Resolving this issue will introduce a major breaking change. package:core package:watchdog type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@ma2ciek
Copy link
Contributor

ma2ciek commented Jan 16, 2020

📝 Provide a description of the new feature

A follow-up to #5891.

With introducing the Context feature the Watchdog should work in the following situations:

  • there is only the Context
  • there are one or multiple editors and the Context
  • there is one editor without the Context

These cases can't be solved with the current Watchdog API or can be done with a lot of boilerplate code that should be hidden from the user.

I'm proposing the following API that introduces the registerSubWatchdogs() method that will handle these cases:

const mainWatchdog = Watchdog.for( Context, contextConfig );
const editorWatchdog1 = Watchdog.for( ClassicEditor, el1, editorConfig1 );
const editorWatchdog2 = Watchdog.for( ClassicEditor, el2, editorConfig2 );

mainWatchdog.registerSubWatchdogs( [ editorWatchdog1, editorWatchdog2 ] );

// to create the context and all editors:
mainWatchdog.create();

// to obtain given editor:
editorWatchdog1.editor;

// to obtain given editor status:
editorWatchdog1.state;

// and later to destory the context and all editors:
mainWatchdog.destroy();

What's also important here is that with the special method like registerSubWatchdogs():

  • restarting process should be easier and hidden from the Watchdog's user
  • restarting only one editor should be available at all - when the context's watchdog is created we can somehow mark/collect these refs and after an error, we can check if an error goes through the area specified before. This way we can ensure that if the error happened in editor1, only the editor1 will be restarted. If the error happened in the Context, then the Context error handler will kill everything and will restart things in the proper order
  • we can have the status of all editors in one place and restart them accordingly with mainWatchdog.restart().
@ma2ciek ma2ciek added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:core package:watchdog labels Jan 16, 2020
@ma2ciek ma2ciek added this to the iteration 29 milestone Jan 16, 2020
@ma2ciek ma2ciek self-assigned this Jan 16, 2020
@ma2ciek
Copy link
Contributor Author

ma2ciek commented Jan 17, 2020

After some internal talks, we changed the API to the one which can be later used in integrations as well.

Usage proposal:

const mainWatchdog = ContextWatchdog.for( Context, contextOptions );

mainWatchdog.add( {
   editor1: {
      type: 'editor',
      creator: () => ClassicTestEditor.create( element1, config1 ),
   },
   annotatedInput: {
      type: 'contextItem',
      creator: () => { }
   }
} );

mainWatchdog.add( {
   editor2: {
      type: 'editor',
      creator: () => ClassicTestEditor.create( element2, config2 ),
   },
} );

await mainWatchdog.getReady();

mainWatchdog.remove( [ 'editor1', 'editor2' ] );

await mainWatchdog.getReady();

await mainWatchdog.destroy();

So here's the API:

// Initialization
const contextWatchdog = ContextWatchdog.for( Context, contextOptions );

// Create items and initialize error handling in them:
contextWatchdog.add( itemConfigs: Object<string, EditorConfig | ContextItemConfig> ): void;

interface EditorConfig {
   type: 'editor';
   creator: () => Promise<Editor>;
   destructor?: ( editor: Editor ) => Promise<void>;
}

interface ContextItemConfig {
   type: 'contextItem';
   creator: () => Promise<ContextItem>;
   destructor?: ( contextItem: ContextItem ) => Promise<void>;
}

// Destroy given items and stop watching them:
contextWatchdog.remove( items: string[] ): void;

// Destroy watchdog with all items:
contextWatchdog.destroy();

// After this action all future `contextWatchdog.add()` should throw.

Angular integration:

<ckeditor
   [editor]="ClassicEditor"
   [(ngModel)]="model1"
   [config]="config1"
   [watchdog]="mainWatchdog">
</ckeditor>

<ckeditor
   [editor]="InlineEditor"
   [config]="config2"
   [data]="data2"
   [watchdog]="mainWatchdog">
</ckeditor>

This way the Angular integration component (<ckeditor>) can run mainWatchdog#add() and mainWatchdog#remove() inside during the lifecycle of the editor component, so the rest should work out of the box. Though it might be tricky to solve the two-way binding case.

This also means that mainWatchdog#remove(); should work even if the mainWatchdog#add() isn't finished yet. (#4696)

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Jan 17, 2020

cc @scofalik, @Reinmar

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Jan 17, 2020

The ContextWatchdog should support also the observable state property. It should have the same (or similar values) as the EditorWatchdog#state and it should be created based on states of all editors in the following way:

  • crashedPermanently - when the reinitialization can't have place 
  • crashed - when one the elements is crashed
  • initializing - only during the initializing
  • destroyed - when the whole ContextEditor is destroyed and it can't be later reused
  • ready - otherwise

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Jan 17, 2020

getReady => waitForReady?

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Jan 22, 2020

After some initial testing of this API I found that the creator: () => ClassicTestEditor.create( element1, config1 ), is unfortunately a no-go. 

During the editor restart, we need to be able to update the editor configuration or data and call the ClassicTestEditor.create() with different args. With the provided API it's impossible as these args are hardcoded in the creator's callback. So we need to change it to something like:

{
   type: 'editor'
   creator: ClassicTestEditor.create.bind( ClassicTestEditor ),
   creationArguments: [ element, config ],
}

Alternatively, the config and element / data could be passed as the named props since they are distinguished later by the EditorWatchdog class anyway.

{
   type: 'editor'
   creator: ClassicTestEditor.create.bind( ClassicTestEditor ),
   config: {},
   sourceElementOrData: '<p>foo</p>'
}

@ma2ciek ma2ciek added the bc:major Resolving this issue will introduce a major breaking change. label Feb 7, 2020
scofalik added a commit to ckeditor/ckeditor5-watchdog that referenced this issue Feb 10, 2020
Feature: Introduced `ContextWatchdog` which is a watchdog for `Context`. Closes ckeditor/ckeditor5#6079. Closes ckeditor/ckeditor5#6042. Closes ckeditor/ckeditor5#4696.

BREAKING CHANGE: The `Watchdog` class was renamed to `EditorWatchdog` and is available in `src/editorwatchdog.js`.
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 )`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bc:major Resolving this issue will introduce a major breaking change. package:core package:watchdog type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant