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

Get rid of soft requirements #2910

Closed
Reinmar opened this issue Aug 27, 2019 · 4 comments · Fixed by ckeditor/ckeditor5-image#326
Closed

Get rid of soft requirements #2910

Reinmar opened this issue Aug 27, 2019 · 4 comments · Fixed by ckeditor/ckeditor5-image#326
Assignees
Labels
package:core status:discussion type:improvement This issue reports a possible enhancement of an existing feature. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Aug 27, 2019

Background: https://github.com/ckeditor/ckeditor5-core/issues/192 and https://github.com/ckeditor/ckeditor5-core/issues/148.

There are two types of plugins which we're not currently listing in requires():

  • Optional ones – those which may change the logic of your plugin when they are loaded, but which are not necessary for your plugin to work.

    Example: if Notification is loaded, use its API to display a warning. Do nothing otherwise.

  • Soft (dynamic) dependencies – those which are in fact required for your plugin to work correctly, but we didn't want to specify which implementation of this functionality we depend on. Instead, a bit like in "program to an interface" we wanted to depend on an interface. The fact that a plugin called 'Clipboard' will be available, but we don't need to assume which instance it is precisely.

The first mechanism works fine. It was improved in https://github.com/ckeditor/ckeditor5-core/issues/148 (plugins.get() throws, so you get the error ASAP).

The second might be improved (with soft requirements – using strings in requires() as initially discussed in https://github.com/ckeditor/ckeditor5-core/issues/148), but it's actually not used by us. I checked @jodator's research (https://github.com/ckeditor/ckeditor5-core/issues/148#issuecomment-445794316) and did some more and we use this approach in very few cases.

Therefore, let's agree that we don't use soft requirements. If you depending on a clipboard, define it by a class in your requirements. This isn't ideal, but this is how reality works, so I have no intention to deny it that :D.

Also, there's always the option to use this technique with NormalModuleReplacementPlugin to inject a different implementation by tuning webpack's configuration. While less convenient, the option exists, so no one will be locked.

What needs to be done?

We need to use requires() in places where it's missing now – this can be found by scanning for plugins.get( ' and filtering out the "optional dependency" cases.

@Reinmar
Copy link
Member Author

Reinmar commented Aug 27, 2019

@jodator, I know you want to do this :D

@jodator jodator self-assigned this Aug 27, 2019
@jodator
Copy link
Contributor

jodator commented Aug 29, 2019

The examples should be swapped - in most cases we do not need Clipboard plugin to work - we only add some functionality to it. So, for me, this is an optional dependency.

The Notification plugin is used as a normal requirement and has proper requires() defined.

Now I'm confused - should we always use hard imports in editor.plugins.get() calls even if its properly required by requires()?

I could only found this one case when plugin used get( 'string' ) that wasn't defined in requires(): https://github.com/ckeditor/ckeditor5-editor-balloon/blob/28ab7bc959f0886c99081c36cc5811b69f2924d2/src/ballooneditorui.js#L51

I found that mostly commands and utility methods use the editor.plugins.get( 'Plugin' ).

@jodator
Copy link
Contributor

jodator commented Aug 29, 2019

Another question: I understand that we must disallow such constructs:

class MyPlugin {
    static get requires() {
        return [ 'Clipboard' ]
    }
}

This construct will work because we map plugin names to constructors and explicitly load them:

https://github.com/ckeditor/ckeditor5-core/blob/5b2507b5494fa26a94d970057fa69aeb9426dbeb/src/plugincollection.js#L259

@jodator
Copy link
Contributor

jodator commented Sep 2, 2019

About that BalloonToolbar plugin use - it is OK. It is not a plugin but a class of and BalloonEditor which always requires BalloonToolbar.

Reinmar referenced this issue in ckeditor/ckeditor5-image Sep 3, 2019
Other: Make the `Clipboard` plugin a required dependency of `ImageUploadEditing`. Closes ckeditor/ckeditor5-core#193.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-core Oct 9, 2019
@mlewand mlewand added this to the iteration 27 milestone Oct 9, 2019
@mlewand mlewand added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. type:task This issue reports a chore (non-production change) and other types of "todos". package:core labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core status:discussion type:improvement This issue reports a possible enhancement of an existing feature. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants