ckeditor5 icon indicating copy to clipboard operation
ckeditor5 copied to clipboard

Destroy plugins in the reverse of the creation order

Open scofalik opened this issue 2 years ago • 1 comments

📝 Provide a description of the improvement

Plugins are instantiated (constructor()) and inited (init()) in the order that is derived from the plugins dependencies and order from config.plugins.

However, when they are destroyed, they are destroyed in the same order. This is incorrect. It is a common practice and a common sense that objects are destroyed in the reverse order of their creation. This way we are sure that one object will not require an already destroyed object for some cleanup purposes.

Following quick test reveals that it is incorrect in our case:

class FooReq {
	constructor() {
		console.log( 'FooReq constr' );
	}

	init() {
		console.log( 'FooReq init' );
	}

	destroy() {
		console.log( 'FooReq destroy' );
	}
}

class Foo {
	static get requires() {
		return [ FooReq ];
	}

	constructor() {
		console.log( 'Foo constr' );
	}

	init() {
		console.log( 'Foo init' );
	}

	destroy() {
		console.log( 'Foo destroy' );
	}
}

ClassicEditor
	.create( '<p>Foo</p>', {
		plugins: [
			Foo,
			FooReq
		],
		toolbar: []
	} )
	.then( editor => {
		editor.destroy();
	} );

Following returns:

FooReq constr
Foo constr
FooReq init
Foo init
FooReq destroy
Foo destroy

While it would be expected to have this order instead:

FooReq constr
Foo constr
FooReq init
Foo init
Foo destroy
FooReq destroy

It seems that it should be enough to change the order in PluginCollection#destroy(). However, it will require some extra testing to make sure that we actually don't break any of our own plugins by fixing this situation. It will be also good to check destroy() methods for all plugins and check the more complicated ones for hidden traps.

scofalik avatar Aug 25 '23 13:08 scofalik

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

CKEditorBot avatar Aug 25 '24 01:08 CKEditorBot

We've closed your issue due to inactivity. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

CKEditorBot avatar Oct 01 '24 23:10 CKEditorBot