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

T/78: Make Plugin an interface (PluginInterface) #84

Merged
merged 10 commits into from
May 23, 2017
Merged

T/78: Make Plugin an interface (PluginInterface) #84

merged 10 commits into from
May 23, 2017

Conversation

pomek
Copy link
Member

@pomek pomek commented May 22, 2017

Suggested merge commit message (convention)

Feature: PluginCollection handles with plugins which do not extend the base Plugin class. Introduced PluginInterface. Closes ckeditor/ckeditor5#2913.

init(), afterInit() and destroy() methods for plugins are optional now.

@pomek pomek requested a review from Reinmar May 22, 2017 06:19
src/plugin.js Outdated
/**
* The base interface for CKEditor plugins.
*
* @interface Plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be an InterfacePlugin?

Copy link
Member Author

@pomek pomek May 22, 2017

Choose a reason for hiding this comment

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

I would call it Plugin and the base plugin class as BasePlugin (which implements the Plugin interface).

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just be consequent with the Plugin name later.

src/plugin.js Outdated
* The editor instance.
*
* @readonly
* @member {module:core/editor/editor~Editor} module:core/plugin~PluginInterface#editor
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to write just @member {module:core/editor/editor~Editor} #editor

*
* @static
* @readonly
* @member {Array.<Function>|undefined} module:core/plugin~PluginInterface.requires
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if an interface might contain static members in JSDoc

Copy link
Member

Choose a reason for hiding this comment

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

For now, JSDoc doesn't support inheriting any static members (jsdoc/jsdoc#1229). We have a plugin for that, fortunately.

src/plugin.js Outdated
* {@link #afterInit} servers for the special "after init" scenarios (e.g. code which depends on other
* plugins, but which doesn't {@link module:core/plugin~PluginInterface.requires explicitly require} them).
*
* @method constructor
Copy link
Contributor

@ma2ciek ma2ciek May 22, 2017

Choose a reason for hiding this comment

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

This method and below methods should start with a #.

@Reinmar
Copy link
Member

Reinmar commented May 22, 2017

OK, so:

@pomek
Copy link
Member Author

pomek commented May 22, 2017

Why couldn't you just merge the #83 into this PR?

we need a test that everything works if you pass a simple function as a plugin (just like in the docs).

https://github.com/ckeditor/ckeditor5-core/pull/83/files#diff-817ad8f70e1fee22c97e006f62a50aeaR201

@pomek
Copy link
Member Author

pomek commented May 22, 2017

init(), afterInit() and destroy() need to be made optional.

It requires changes in the Editor class. Will do it tomorrow.

@pomek
Copy link
Member Author

pomek commented May 23, 2017

@Reinmar, I guess it's ready to review.

@Reinmar
Copy link
Member

Reinmar commented May 23, 2017

gulp test --files=ui,undo throws on this branch.

@pomek
Copy link
Member Author

pomek commented May 23, 2017

Undo works fine.

@pomek
Copy link
Member Author

pomek commented May 23, 2017

gulp test --files=ui/toolbar/contextual/contextualtoolbar.js,ui/panel/balloon/balloonpanelview.js

also works fine :|

@pomek
Copy link
Member Author

pomek commented May 23, 2017

And gulp test --files=ui also works fine.

I don't understand what happens.

@Reinmar
Copy link
Member

Reinmar commented May 23, 2017

Some tests affect other tests. Have fun debugging ;)

@pomek
Copy link
Member Author

pomek commented May 23, 2017

I guess the tests fail because some of the plugins call super.destroy() that does not exist in the base Plugin class.

@Reinmar
Copy link
Member

Reinmar commented May 23, 2017

Makes sense. So, let's add empty destroy() to Plugin, because we plan to add it anyway soon (https://github.com/ckeditor/ckeditor5-core/issues/86).

@pomek
Copy link
Member Author

pomek commented May 23, 2017

@Reinmar, ui and undo work fine for me. Could you confirm?

@Reinmar Reinmar merged commit f476f34 into master May 23, 2017
@Reinmar Reinmar deleted the t/78b branch May 23, 2017 09:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Plugin an interface
3 participants