search

ckeditor / ckeditor5

Powerful rich text editor framework with a modular architecture, modern integrations, and features like collaborative editing.
https://ckeditor.com/ckeditor-5
Other
9.37k stars 3.68k forks source link

Allow adding plugins to a CKEditor 5 build #667

Closed pjasiun closed 3 years ago

pjasiun commented 6 years ago

Right now there is no easy way to add a real plugin to the CKEditor 5 build. Even if it is theoretically possible, you will not be able to create a position or fire an event.

The problem is that build has all classes build in and expose only editors API. If a custom plugin imports Position from the ckeditor5-engine and use it in the plugin it will not be the same instance as the one used in the engine in the build. Because of it, this position will return false when position instanceof Position will be called, so the user will get an ugly bug.

There is a simmilar problem with symbols, which are used in the events system.

Note that it is even worse because the same issue will appear when a custom plugin will have a different version of the engine on the list of dependencies. We could say that in such case semantic versioning will save us, but it still means that with every major engine release all 3rd party plugins will stop working immediately, even if changes in the engine breaks nothing important for them. The version of the dependency will not match anymore and 2 separate engines will be included with not matching classes.

This very ticky problem, but there are some improvements we could think about.

Since editor usually needs all engine features, we could expose them as document property, have something like document.createPosition. Then all plugins will use instances of the same class and the problem disappears. In fact, when all engine utiles will be exposed on the document model (or controller), plugins will not need to depends on the engine and only use engine exposed by the editor.

Unfortunately, it looks fine only in the theory. Only for position we have:

Position.constructor( root, path )
Position.createAt( itemOrPosition, offset )
Position.createAfter( item )
Position.createBefore( item )
Position.createFromParentAndOffset( parent, offset )
Position.createFromPosition( position )

Then there is range:

Range.constructor( start, end )
Range.createFromPositionAndShift( position, shift )
Range.createFromParentsAndOffsets( startElement, startOffset, endElement, endOffset )
Range.createFromRange( range )
Range.createIn( element )
Range.createOn( item )
Range.createCollapsedAt( itemOrPosition, offset )
Range.createFromRanges( ranges )

Then, treeWalker... But hey! It's only the model. We have also all conversion utils to be exposed because one will need them for sure. And working with converters one will need to be able to use view API, so multiple constructors in the view need to be exposed too. It means a lot of factories and a lot of ugly APIs. And I'm talking here only about the engine, but note that there is the same problem with utils and UI. The additional problem with utils is that we can not expose all of them because we don't know which are really needed.

Another solution would be to stop using instanceof and symbols since they are problematic. We would have to have is method in each class or use duck typing. Symbols could be replaced with unique strings. It would not solve the problem of duplicated dependencies, the build will be bigger but the editor will work fine.

I think that we should end up with something in between. We may accept that some utils will be duplicated as long as they don't break the editor, but we should ensure that only one engine will be loaded.

pjasiun commented 6 years ago

Talking about exposing engine elements note, that some of them are already done this way (for instance markers are available only through markers collection) and after https://github.com/ckeditor/ckeditor5-engine/issues/858 all model elements will have to be created through batch API, and all view elements through view writer.

Reinmar commented 6 years ago

We need to find a good balance between exposing everything and ultimately allowing to write every kind of feature without building the editor and exposing too little and making writing typical features impossible.

E.g. with UI, it's rather impossible for us to expose all UI components. We may expose some, but then people will ask why not the other ones and it will be confusing. So, let's maybe make it possible to duplicate UI's code in a way that it won't blow up. But then... PostCSS will be a problem (duplicated styles), so I guess there's just no option.

With the conversion we can perhaps try to define some std set of tools which make it possible to write most basic features. This should be more understandable.

With tree walkers and stuff – they are already available from ranges, so that's not a problem.

Etc, etc. We need to look at every case and see if there's a simple and reasonable solution. If not, then let's just state that a certain thing can only be done by rebuilding the editor. We may also create a guide which explains this.

Reinmar commented 6 years ago

Another question about this: https://stackoverflow.com/questions/49358150/is-there-any-way-to-select-all-the-content-of-ckeditor-5-by-code/49358509#49358509

Here, the requirement is to have access to createRange*.

Those questions repeat all the time. So, let's simply write it down here what developers have missed. So far, I saw that lack of createPosition* and createRange* is the biggest issue.

oleq commented 6 years ago

This asks for an entry in FAQ.

Reinmar commented 6 years ago

I don't know what we could write there, expect warning that adding plugins without rebuilding the editor might not be possible. You should not install e.g. @ckeditor/ckeditor5-engine next to @ckeditor/ckeditor5-build-classic and import things from both packages.

Reinmar commented 6 years ago

This CI check will help a bit in verifying how much we depend on ckeditor5-engine: https://github.com/ckeditor/ckeditor5-dev/issues/392

From my checks, ckeditor5-heading does not depend on the engine any more, and ckeditor5-highlight only on the range. So, again, createRange() and createPosition() will be awesome :P

Reinmar commented 6 years ago

Another issue caused, most likely, by package duplication: https://github.com/ckeditor/ckeditor5/issues/957

There are a couple more things I'm considering:

Reinmar commented 6 years ago

One thing I like in the fact that feature packages need to depend on @ckeditor/ckeditor5-core is that:

That may be a counterargument for getting rid of Plugin and Command imports.

Reinmar commented 6 years ago

Last but not least, our CKEditorWebpackPlugin may be able to warn developers about broken installations (duped packages) and perhaps even force webpack to use e.g. a single ckeditor5-engine version. But I guess that this will be tricky hackery. cc @ma2ciek

ma2ciek commented 6 years ago

I think it's not so hard to warn developers about duplicates. And it's possible to force webpack to use one version of some package using e.g https://webpack.js.org/configuration/resolve/#resolve-alias. But that's on the developer side. From our side, it's also doable, but harder and dependent on the low-level webpack stuff. But either way, we would need to know where correct packages are while resolving imports and that might be tricky though.

Reinmar commented 6 years ago

I imagine we could, perhaps, record all resolve paths, store them and once detecting that someone tries to resolve the same module but in a different package path, use the stored package path.

ma2ciek commented 6 years ago

I think, that it might be worth checking whether versions of these packages are the same. I imagine somebody might have:

node_modules
  - @ckeditor/ckeditor5-engine@1.0.0-alpha.1
  - @ckeditor/ckeditor5-build-classic
     - node_modules
        @ckeditor/ckeditor5-engine@1.2.0

and import stuff from both packages. Then the approach to just take first found package and reuse it might break the build.

Reinmar commented 6 years ago

Another thing regarding duplicated pkgs:

https://gitter.im/ckeditor/ckeditor5?at=5ad59c017c3a01610deddd3e

The other problem that you have is that you're trying to use a build + import some source modules (the Plugin class). Please note that in https://docs.ckeditor.com/ckeditor5/latest/framework/guides/quick-start.html we don't use the build, but the editor class itself.

OTOH, I have no idea why that would case the "class constructors must be invoked with new" ;| CKEditor does use new when initialising all plugins

If we'd avoid the need to use the Plugin class completely (and, to make it consistent, the Command class too), we wouldn't have such issues... I'm starting to seriously consider this. Especially, that it would not be a breaking change. The only problem would be with the documentation.

Reinmar commented 6 years ago

I've got a brilliant idea :trollface:. We want to allow writing plugins without including @ckeditor/ckeditor-core into your dependencies. At the same time, we want to allow defining with which version of CKEditor 5 a plugin will work with.

So far, we've done that by forcing everyone to import many classes from @ckeditor/ckeditor5-engine, @ckeditor/ckeditor5-core and @ckeditor/ckeditor5-ui because that seemed to be the semver/npm way. Unfortunately/fortunately, npm duplicates packages if there are conflicts in dependency versions. This leads to problems, instead of helping in anything because the developer does not know about conflicts (until the editor blows up). What's more, the developer may use some 3rd party plugins which might've not had any updates for some time and hence, may depend on old versions of core packages. This will lead to a deadlock because that developer will not be able to use those 3rd party plugins, while there's actually a quite high chance that they would work (because the API doesn't change that drastically with every major version).

So, it's only reasonable to abandon the idea that npm/semver may help us here. My idea is that we can both enable people to write the most type of plugins without depending on any of our core packages, and to express with which version of CKEditor 5 that plugin worked. How to do that?

  1. Let's first get rid of dependencies on the engine, core and UI from most features. This was discussed earlier in this ticket.
  2. Then, remove those dependencies from the features which don't need them.

  3. Finally, add ckeditor5 as a peer dependency of all these features:

    In some cases, you want to express the compatibility of your package with a host tool or library, while not necessarily doing a require of this host. This is usually referred to as a plugin. Notably, your module may be exposing a specific interface, expected and specified by the host documentation.

    Trying to install another plugin with a conflicting requirement will cause an error. For this reason, make sure your plugin requirement is as broad as possible, and not to lock it down to specific patch versions.

    Source: https://docs.npmjs.com/files/package.json#peerdependencies

I'm not 100% sure yet how peer deps work, but one downside I see is that people will have to remember to install ckeditor5, otherwise npm will complain. However, this should be quite ok.

What will ckeditor5 contain? Nothing. A readme and license files. It's just a way to synchronise multiple packages. A virtual platform. An ecosystem :D

pjasiun commented 6 years ago

:+1: I am not sure about details, how we will be able to get rid of ckeditor5-ui and ckeditor5-utils imports and I am not sure how peer dependencies will help us, but I fully agree with:

So, it's only reasonable to abandon the idea that npm/semver may help us here.

Reinmar commented 6 years ago

Another case, which, unfortunately, I don't see how we could fix: https://github.com/ckeditor/ckeditor5-markdown-gfm/issues/22

Reinmar commented 6 years ago

One more idea, log a warning when we discover that someone added plugins to an already existing build. We can fairly easy discover that some new constructors were passed to config.plugins. We could document this issue clearly then.

However, this would have a very negative side effect – you wouldn't be able to write simple, dependency-less plugins at all. So that would actually work in the other direction than asked in this ticket – you wouldn't be able to add any new functionality to a build.

Back to the drawing board...

Reinmar commented 6 years ago

I extracted the idea to make ckeditor5 a peer dep of other packages to https://github.com/ckeditor/ckeditor5/issues/1061.

Reinmar commented 6 years ago

Unsurprisingly, the problem with duplicated dependencies isn't only touching us. I've stumbled upon https://github.com/facebook/draft-js/issues/1763 which links to an interesting https://stackoverflow.com/questions/26737819/why-use-peer-dependencies-in-npm-for-plugins/34645112#34645112.

Reinmar commented 6 years ago

And there's also an interesting bit in this:

https://medium.com/@penx/managing-dependencies-in-a-node-package-so-that-they-are-compatible-with-npm-link-61befa5aaca7

It links to https://github.com/darrenscerri/duplicate-package-checker-webpack-plugin – something that we actually wanted :)

ma2ciek commented 6 years ago

It links to darrenscerri/duplicate-package-checker-webpack-plugin – something that we actually wanted :)

It sounds really good. The only missing thing here is some resolution/deduplication of duplicated files. But maybe we can force developers to use aliases then (https://github.com/ckeditor/ckeditor5/issues/667#issuecomment-380718442).

Reinmar commented 6 years ago

I think that resolving such situations needs to be done like this:

pjasiun commented 6 years ago

Yesterday I was adding plugin, command, and button to the build. Everything was working until I added:

import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';

This import was enough to make editor stop working (I even did not need to use it). It was a little bit strange (especially because other imports like Plugin or Command were working fine), but not that much, considering issues we have described in this thread.

ma2ciek commented 6 years ago

The problem, I see, is that bundled build still won't work if we have some webpack solution, because all imports will be already resolved in the build.

It would help only for the source imports:

import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';
import ClassicEditorBuild from '@ckeditor/ckeditor5-build-classic/src/ckeditor';

The duplicated stuff can be deduped here.

Reinmar commented 6 years ago

It would help only for the source imports:

Exactly. We're not able to dedupe this in other cases. And, in fact, if you used CKEditor 5 source and and have a clean installation, then there's no duplication – npm installs a flat tree of pkgs. So there's nothing to fix in this situation.

Duplication happens in two cases:

If we'll stop using instanceof and fix some code, we'll avoid crashes in both these scenarios. But in both this cases duplication will lead to awfully big bundles, so we don't want people to do that. Hence, it'd be good to warn them in both cases.

In the former case, we can use some webpack tool for that (or implement it in our plugin).

In the latter, we'll need something like I described in https://github.com/ckeditor/ckeditor5/issues/1005:

BTW, we can add a check. If window.CKEDITOR_VERSION is defined when this module will be executed, we can log a warning that either someone loaded two builds on one page. Or that packages got duplicated while installation (which might be caused by mismatched dependency versions).

pjasiun commented 6 years ago

One idea just comes to my mind. Since the main problem here is using instances of 2 identical classes (instanceof does not work then), maybe our classes should be exported using kind of the singleton pattern.

Something like:

class Position {
    // ...
}

if ( !window.CKEDITOR.ENGINE_MODEL_POSITION ) {
    window.CKEDITOR.ENGINE_MODEL_POSITION = Position;
}

export default Position = window.CKEDITOR.ENGINE_MODEL_POSITION;

Of course, we could hide the ugly part in some utils. And, as much as I hate global variables and singleton partter it might solve the problem with prototyping some plugins using already build version of the editor.

jodator commented 6 years ago

One idea just comes to my mind. Since the main problem here is using instances of 2 identical classes (instanceof does not work then), maybe our classes should be exported using kind of the singleton pattern.

It is so FUGLY... I'd rather see that we explore all other options rather then make this singleton.

Other idea (similar to above) is to have methods that will return proper classes that other plugins might use but it also isn't nice:

const Postion = editor.model.Position

So a special getter in model for engine:model/position or...

const Position = editor.getClass( 'model.Position')

...but those are ugly too :(

Reinmar commented 6 years ago 
import { define } from 'ckeditor5';

class SomeClass {}

export default define( 'core/someclass~SomeClass', SomeClass );

In some other place:

const SomeClass = Editor.import( 'core/someclass~SomeClass' );

Where:

// @ckeditor/ckeditor5-core/src/editor/editor.js

import { import } from 'ckeditor5';

export default class Editor {}

Editor.import = import;

But at the same time this will work too:

import SomeClass from '@ckeditor/ckeditor5-core/src/someclass';

And where define() and import() look like this:

// ckeditor5/index.js

window.CKEDITOR_MODULES = {};

export function define( exportName, exportValue ) {
    if ( !window.CKEDITOR_MODULES[ exportName ]  ) {
        window.CKEDITOR_MODULES[ exportName ] = exportValue;
    }

    return window.CKEDITOR_MODULES[ exportName ];
}

export function import( exportName ) {
    if ( !window.CKEDITOR_MODULES[ exportName ]  ) {
        throw new CKEditorError();
    }

    return window.CKEDITOR_MODULES[ exportName ];
}

So you'll also be able to do this when hacking some demos:

const SomeClass = window.CKEDITOR_MODULES[ 'core/someclass~SomeClass' ];
Reinmar commented 6 years ago

It could also be a single module() function which could be used like this:

import { module } from 'ckeditor5';

module( 'module/path~exportName', ExportThing ); // -> ExportThing

module( 'module/path~exportName' ); // -> ExportThing
Reinmar commented 6 years ago

NOTE: This will prevent errors caused by package duplication and will allow prototyping things fast when using a build (because many modules, but not all, will be registered in window.CKEDITOR_MODULES). However, this doesn't prevent code duplication per se and doesn't reduce the need for depending on ckeditor5-engine, ckeditor5-core, etc when writing plugins. So, it's a part of a bigger puzzle.

pjasiun commented 5 years ago

I was thinking about it for a while after we recently closed https://github.com/ckeditor/ckeditor5-engine/issues/1555 and https://github.com/ckeditor/ckeditor5-engine/issues/1556.

Still, the main issue is duplicated imports: when some plugins requirements are part of the editor build, but also loaded "outside the editor build" there are two instances of the same module in a single scope. The same when two modules use a different version of dependencies: even if there are not critical differences, two versions of the same module will be loaded what will cause even more issues.

I believe that using the method described above we should be able to solve it. Thanks to it, only a single module will be available in the scope what is expected. Note that if 2 plugins expect dependencies in different versions only one of them will be loaded, so it might happen that one of them will not get the API it would like to have. However, I think it is understandable when you load mismatch plugin you may have outdated API.

The other issue is CSS import, which also should be loaded only once. This can be solved using require. I've just tested it and it works well with the webpack configuration we use (it is fine to mix imports and require in webpack):

if ( window.loadCSS ) {
    require( './style.css' );
}

It can be also solved using dynamically import:

if ( window.loadCSS ) {
    import( './style.css' );
}

It also works out of the box with our webpack configuration, however, this way a separate chunk will create.

Note that webpack chunks is an interesting topic. It can slove also the problem of the code getting bigger when some modules are loaded twice, see prevent duplication chapter.

pjasiun commented 5 years ago

In the F2F talk, we agreed that the minimum we could do is to log a warning when a key module (engine.docment, ui.view, domemittermixin?) is loaded twice in the same scope (window).

pjasiun commented 5 years ago

I checked the number of imports "from @ckeditor`. I excluded editor creators, builds and essential plugin since these are different cases.

What I get is:

102 times:

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';

31 times:

import Command from '@ckeditor/ckeditor5-core/src/command';

76 imports from the editor-ui (23 deduplicate):

import BalloonPanelView from '@ckeditor/ckeditor5-ui/src/panel/balloon/balloonpanelview';
import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';
import clickOutsideHandler from '@ckeditor/ckeditor5-ui/src/bindings/clickoutsidehandler';
import ComponentFactory from '@ckeditor/ckeditor5-ui/src/componentfactory';
import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/panel/balloon/contextualballoon';
import FocusCycler from '@ckeditor/ckeditor5-ui/src/focuscycler';
import IconView from '@ckeditor/ckeditor5-ui/src/icon/iconview';
import InputTextView from '@ckeditor/ckeditor5-ui/src/inputtext/inputtextview';
import LabeledInputView from '@ckeditor/ckeditor5-ui/src/labeledinput/labeledinputview';
import Model from '@ckeditor/ckeditor5-ui/src/model';
import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification';
import preventDefault from '@ckeditor/ckeditor5-ui/src/bindings/preventdefault';
import SplitButtonView from '@ckeditor/ckeditor5-ui/src/dropdown/button/splitbuttonview';
import submitHandler from '@ckeditor/ckeditor5-ui/src/bindings/submithandler';
import Template from '@ckeditor/ckeditor5-ui/src/template';
import ToolbarSeparatorView from '@ckeditor/ckeditor5-ui/src/toolbar/toolbarseparatorview';
import ToolbarView from '@ckeditor/ckeditor5-ui/src/toolbar/toolbarview';
import View from '@ckeditor/ckeditor5-ui/src/view';
import ViewCollection from '@ckeditor/ckeditor5-ui/src/viewcollection';
import { addListToDropdown, createDropdown } from '@ckeditor/ckeditor5-ui/src/dropdown/utils';
import { createDropdown } from '@ckeditor/ckeditor5-ui/src/dropdown/utils';
import { createDropdown, addListToDropdown } from '@ckeditor/ckeditor5-ui/src/dropdown/utils';
import { createDropdown, addToolbarToDropdown } from '@ckeditor/ckeditor5-ui/src/dropdown/utils';

254 imports from utiles (43 deduplicated):

import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
import Collection from '@ckeditor/ckeditor5-utils/src/collection';
import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';
import Config from '@ckeditor/ckeditor5-utils/src/config';
import count from '@ckeditor/ckeditor5-utils/src/count';
import diff from '@ckeditor/ckeditor5-utils/src/diff';
import diffToChanges from '@ckeditor/ckeditor5-utils/src/difftochanges';
import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin';
import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
import env from '@ckeditor/ckeditor5-utils/src/env';
import fastDiff from '@ckeditor/ckeditor5-utils/src/fastdiff';
import first from '@ckeditor/ckeditor5-utils/src/first';
import FocusTracker from '@ckeditor/ckeditor5-utils/src/focustracker';
import getAncestors from '@ckeditor/ckeditor5-utils/src/dom/getancestors';
import getCommonAncestor from '@ckeditor/ckeditor5-utils/src/dom/getcommonancestor';
import global from '@ckeditor/ckeditor5-utils/src/dom/global';
import indexOf from '@ckeditor/ckeditor5-utils/src/dom/indexof';
import insertAt from '@ckeditor/ckeditor5-utils/src/dom/insertat';
import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable';
import isNode from '@ckeditor/ckeditor5-utils/src/dom/isnode';
import isRange from '@ckeditor/ckeditor5-utils/src/dom/isrange';
import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
import KeystrokeHandler from '@ckeditor/ckeditor5-utils/src/keystrokehandler';
import Locale from '@ckeditor/ckeditor5-utils/src/locale';
import log from '@ckeditor/ckeditor5-utils/src/log';
import mix from '@ckeditor/ckeditor5-utils/src/mix';
import objectToMap from '@ckeditor/ckeditor5-utils/src/objecttomap';
import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
import priorities from '@ckeditor/ckeditor5-utils/src/priorities';
import Rect from '@ckeditor/ckeditor5-utils/src/dom/rect';
import remove from '@ckeditor/ckeditor5-utils/src/dom/remove';
import setDataInElement from '@ckeditor/ckeditor5-utils/src/dom/setdatainelement';
import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
import toUnit from '@ckeditor/ckeditor5-utils/src/dom/tounit';
import uid from '@ckeditor/ckeditor5-utils/src/uid';
import uid from '@ckeditor/ckeditor5-utils/src/uid.js';
import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard';
import { getCode, keyCodes, parseKeystroke } from '@ckeditor/ckeditor5-utils/src/keyboard';
import { getEnvKeystrokeText } from '@ckeditor/ckeditor5-utils/src/keyboard';
import { getOptimalPosition } from '@ckeditor/ckeditor5-utils/src/dom/position';
import { isInsideSurrogatePair, isInsideCombinedSymbol } from '@ckeditor/ckeditor5-utils/src/unicode';
import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
import { scrollViewportToShowTarget } from '@ckeditor/ckeditor5-utils/src/dom/scroll';

17 imports svg:

import cancelIcon from '@ckeditor/ckeditor5-core/theme/icons/cancel.svg';
import cancelIcon from '@ckeditor/ckeditor5-core/theme/icons/cancel.svg';
import cancelIcon from '@ckeditor/ckeditor5-core/theme/icons/cancel.svg';
import centerIcon from '@ckeditor/ckeditor5-core/theme/icons/object-center.svg';
import checkIcon from '@ckeditor/ckeditor5-core/theme/icons/check.svg';
import checkIcon from '@ckeditor/ckeditor5-core/theme/icons/check.svg';
import checkIcon from '@ckeditor/ckeditor5-core/theme/icons/check.svg';
import fullSizeIcon from '@ckeditor/ckeditor5-core/theme/icons/object-center.svg';
import fullWidthIcon from '@ckeditor/ckeditor5-core/theme/icons/object-full-width.svg';
import iconPilcrow from '@ckeditor/ckeditor5-core/theme/icons/pilcrow.svg';
import imageIcon from '@ckeditor/ckeditor5-core/theme/icons/image.svg';
import leftIcon from '@ckeditor/ckeditor5-core/theme/icons/object-left.svg';
import pencilIcon from '@ckeditor/ckeditor5-core/theme/icons/pencil.svg';
import quoteIcon from '@ckeditor/ckeditor5-core/theme/icons/quote.svg';
import rightIcon from '@ckeditor/ckeditor5-core/theme/icons/object-right.svg';
import sideIcon from '@ckeditor/ckeditor5-core/theme/icons/object-right.svg';
import textAlternativeIcon from '@ckeditor/ckeditor5-core/theme/icons/low-vision.svg';

31 imports from engine:

import bindTwoStepCaretToAttribute from '@ckeditor/ckeditor5-engine/src/utils/bindtwostepcarettoattribute';
import ClickObserver from '@ckeditor/ckeditor5-engine/src/view/observer/clickobserver';
import Conversion from '@ckeditor/ckeditor5-engine/src/conversion/conversion';
import DataController from '@ckeditor/ckeditor5-engine/src/controller/datacontroller';
import DomConverter from '@ckeditor/ckeditor5-engine/src/view/domconverter';
import DomConverter from '@ckeditor/ckeditor5-engine/src/view/domconverter';
import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';
import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';
import DomEventObserver from '@ckeditor/ckeditor5-engine/src/view/observer/domeventobserver';
import DowncastHelpers from '@ckeditor/ckeditor5-engine/src/conversion/downcasthelpers';
import EditingController from '@ckeditor/ckeditor5-engine/src/controller/editingcontroller';
import Element from '@ckeditor/ckeditor5-engine/src/view/element';
import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/htmldataprocessor';
import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/htmldataprocessor';
import LivePosition from '@ckeditor/ckeditor5-engine/src/model/liveposition';
import LiveRange from '@ckeditor/ckeditor5-engine/src/model/liverange';
import Matcher from '@ckeditor/ckeditor5-engine/src/view/matcher';
import Model from '@ckeditor/ckeditor5-engine/src/model/model';
import MouseObserver from '@ckeditor/ckeditor5-engine/src/view/observer/mouseobserver';
import Observer from '@ckeditor/ckeditor5-engine/src/view/observer/observer';
import Observer from '@ckeditor/ckeditor5-engine/src/view/observer/observer';
import Observer from '@ckeditor/ckeditor5-engine/src/view/observer/observer';
import UpcastHelpers from '@ckeditor/ckeditor5-engine/src/conversion/upcasthelpers';
import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter';
import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter';
import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter';
import ViewMatcher from '@ckeditor/ckeditor5-engine/src/view/matcher';
import { attachPlaceholder } from '@ckeditor/ckeditor5-engine/src/view/placeholder';
import { getFillerOffset } from '@ckeditor/ckeditor5-engine/src/view/containerelement';
import { NBSP_FILLER } from '@ckeditor/ckeditor5-engine/src/view/filler';
import { transformSets } from '@ckeditor/ckeditor5-engine/src/model/operation/transform';

13 imports from widget:

import Widget from '@ckeditor/ckeditor5-widget/src/widget';
import Widget from '@ckeditor/ckeditor5-widget/src/widget';
import Widget from '@ckeditor/ckeditor5-widget/src/widget';
import WidgetToolbarRepository from '@ckeditor/ckeditor5-widget/src/widgettoolbarrepository';
import WidgetToolbarRepository from '@ckeditor/ckeditor5-widget/src/widgettoolbarrepository';
import WidgetToolbarRepository from '@ckeditor/ckeditor5-widget/src/widgettoolbarrepository';
import { findOptimalInsertionPosition } from '@ckeditor/ckeditor5-widget/src/utils';
import { findOptimalInsertionPosition } from '@ckeditor/ckeditor5-widget/src/utils';
import { findOptimalInsertionPosition, isWidget, toWidget } from '@ckeditor/ckeditor5-widget/src/utils';
import { isWidget, toWidget } from '@ckeditor/ckeditor5-widget/src/utils';
import { isWidget, toWidget } from '@ckeditor/ckeditor5-widget/src/utils';
import { toWidgetEditable } from '@ckeditor/ckeditor5-widget/src/utils';
import { toWidgetEditable } from '@ckeditor/ckeditor5-widget/src/utils';

16 imports of plugins from other modules:

import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions';
import PendingActions from '@ckeditor/ckeditor5-core/src/pendingactions';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard';
import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard';
import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard';
import Undo from '@ckeditor/ckeditor5-undo/src/undo';
import Undo from '@ckeditor/ckeditor5-undo/src/undo';
import Image from '@ckeditor/ckeditor5-image/src/image';
import ImageUpload from '@ckeditor/ckeditor5-image/src/imageupload';

1 special (snowflake) import:

import FileDialogButtonView from '@ckeditor/ckeditor5-upload/src/ui/filedialogbuttonview';

I believe each of these groups need to be handled differently.

jodator commented 5 years ago

That's a ton and even in groups some of the imports might be handled differently (FileRepository vs Undo). Also this looks like massive work and I'm not sure that everything is needed. Since some code duplication couldn't be avoided (like utils imports). Also not everything must be on the same version (utils most likely) but others do - to have the same API (ie. Widget, ui classes).

Probably some of the Paragraph or other might be removed and used as strings in Plugin.requires() as I suspect that most of this group is used as this.

pjasiun commented 5 years ago

Since some code duplication couldn't be avoided (like utils imports).

Fully agree. Also, I believe that some of these imports are basically fine. However, all of them should be analyzed to have a final solution.

pjasiun commented 5 years ago

I have analyzed these imports and the problems we have and I have some conclusions.

Problems we will not be able to solve

Let's start from the sad part, problems we will not be able to solve. To make it clear.

Size of builds

If we let users add plugins to build and these plugins will have duplicate dependencies (for instance have dependencies which are already loaded in the editor build), the total size of the editor will be bigger. Even if these duplicated dependencies will not be used at all, they still will be there and increase the size of the editor. The only way to have the smallest possible build will always be building all modules together and letting webpack to handle package deduplication.

Problems if plugins are in not compatible versions

If the plugin expects a different version of the API than the core editor exposes and the API is not backward compatible it will not work, basically. Also if two plugins which should work together are in not compatible versions they will not work either. We can provide a proper error and limit this issue to rare backward incompatible, but it will always occur from time to time. I believe this is the common problem of all pluggable systems.

The problem we can solve

The problem we can solve is: how complicated it is to add plugins to the editor. The number of reported bug and the fact that ckeditor-duplicated-modules is one of the most popular error pages proves it is the problem. It is hard to even list here all possible issues people have. Adding plugins, prototyping custom plugins, adding adapters (upload, comments, etc.), should be simpler. Now the developer needs to know what he can do depending on the path of adding plugin he has chosen. The number of limitation when you want to add a plugin to the build is so tricky that even people who are with the project from the beginning are doing mistakes. It is also a kind of a paper cut issue - it is usually not very hard to fix but this is the issue you meet at the very early stage of working with the project, and it makes a very bad first impression. It also makes prototyping much more inconvenient.

It would be great if one day it will be possible to make this work:

<!DOCTYPE html>
<html>
<head>
    <title>CKEditor 5 with some plugin</title>
    <script src="ckeditor5.js"></script>
    <script src="somePlugin.js"></script>
</head>
<body>
    <div class="editor"></div>
    <script>
        ClassicEditor.create( document.querySelector( '.editor' ), {
            extraPlugins: [ SomePlugin ]
        } );
    </script>
</body>
<html>

How?

So: what can we do? I think this is not is a good stage to find the full solution. There are too many issues to see the full picture. However, there are some steps we should be able to do to limit the number of problems. Then, we will see if we need and can do something more.

Expose more API

Exposing API on the editor class is something we already started. Many constructors are already exposed and do not need to be imported. However, I am sure we can expose more.

Make sure that double imports of utils/helpers do not cause issues

I do not think that everything should be exposed. There is no need to expose uid util - nothing bad will happen if 2 plugins import it, even if such import will not be deduplicated and the function will be loaded twice.

However, I think that much more classes should work this way (we should allow developers to import them multiple times and make sure it works fine).

To achieve this, we should for sure get rid of instanceof and Symbols. instanceof can be replaced by custom methods or duck typing.

Here is the number of usages in the project, excluding tests:

Symbol:
43 matches across 15 files

instanceof:
187 matches across 56 files

Comparison of solutions presented above

So we have 2 solutions: "exposed API" and "utils-like API". The disadvantage of "utils-like API" is that it makes the build bigger if two modules load it. This is why I think that "utils-like API" should not have any bigger logic.

The disadvantage of "exposed API" is that it will be loaded always, does not matter if it is needed or not. Also, it needs to be exposed on some object passed to each module. Usually, it is editor instance, but for classes like Plugins it is tricky how it could be passed.

I think that we should use one of these methods for all classes "imported" by plugins, with 2 exceptions mentioned below.

Make sure that styles are not loaded twice

The first exception are styles. They are always loaded to the global scope and we know that issues appear when they are loaded twice. We should make sure they are loaded only once:

if ( !window.style1Loaded ) {
    window.style1Loaded = true;
    require( './style1.css' );
} else {
    console.warn( 'Styles tried to be loaded twice' );
}

It is hard to say what should happen if a style is required twice and if preventing it solves any issue. For instance, on the one hand, it may allow users to run multiple different editors on the same page, on the other hand, some other styles might have conflicts anyway and it might cause problems similar to the one described at the beginning ("Problem if plugins are in not compatible versions"). I believe that some warning if styles suppose to be loaded twice is a good solution for now: it will warn developers that something might be wrong, but do not stop running application.

Import plugins as dependencies

The second special case are plugins which might be dependencies of other plugins. Here we have also room for improvements:

To help developers solve some problems we could introduce API versioning for plugins which are made to be imported by other plugins, like FileRepository or PendingActions. They should have not only name and also version property:

export default class PendingActions extends Plugin {
    static get pluginName() {
        return 'PendingActions';
    }

    static get pluginAPIVersion() {
        return 42;
    }

    //...

Then, if two modules loaded PendingActions plugin and they are bundled separately, we can check if they use the same plugin API version. If the API version does not match, we can throw an exception. Possibly, we can use package.json version as the plugin version.

import { version } from '../package.json'

export default class PendingActions extends Plugin {
    static get pluginName() {
        return 'PendingActions';
    }

    static get pluginAPIVersion() {
        return version;
    }

This could be even done in the base Plugin class so all plugins which extends Plugin will have version and let us solve double imports in the more secure way.

Disclaimer

All of the above, are just drafts, proposals. Before we will start working on anything we need to create separate tickets and specify them better. However, first, we need to agree on the general way how we want to address this issue.

Finally, for sure we will not be able to do it as a backward compatible change.

oleq commented 5 years ago

Just to let you know, style duplication has catastrophic consequences because in CSS the order of parsing has an impact on style specificity. Duplicated styles override adjustments to original styles and totally break the layout.

The problem with loading styles is that it is automated and happens in separate threads.

When webpack traverses the JS codebase, it finds CSS imports and uses postcss-loader to process it. Because each package imports its own styles, each such import is a "root import", which means that files in ckeditor5-basic-styles are processed in a different PostCSS thread than those of ckeditor5-link. Those are different entry points for styles.

As a result, processing ckeditor5-basic-styles has no knowledge about imports in ckeditor5-link. And vice–versa.

Then the style-loader takes over and automatically injects a script into a build that renders <style> tags in DOM and fills them in with the output of postcss-loader.

First things first, I see no way to inject any logic between postcss-loader and style-loader to determine what should be loaded and what shouldn't. Secondly, people may want to extract CSS to a separate file rather than keep them in the bundle, which means we cannot use style-loader for deduping because it is not a mandatory part of the chain.

Writing a PostCSS plugin for that isn't a good option either because as I mentioned earlier, there are multiple threads of PostCSS engaged in the building process and they cannot communicate with each other. Maybe we could use a plain text file to store information about processed CSS files so the following (or parallel) PostCSS process can read it and tell what will duplicate. But this is a maybe.

A custom loader for webpack could help. Not sure how exactly, but I guess it would act as a post-processor, after postcss-loader.

A possible problem: there are CSS imports inside CSS files which webpack cannot reach or parse. Webpack handles imports inside JS files and if a JS files imports a CSS file which, on the other hand, imports another CSS file (buttonview.js -> button.css -> some-helpers.css), there's no change to dedupe the last one. OTOH, we wrote the codebase in such a way that derivative imports (some-helpers.css) bring no renderable content, for instance, only mixins. So maybe there's some chance this could work if we learned how to write webpack loaders properly :)

ma2ciek commented 5 years ago

IMO Plugins are often used as singletons so we can't assume that loading them twice won't be a problem. And I think that the root of this problem lies in our versioning system too, so I'd focus on this problem as well: https://github.com/ckeditor/ckeditor5/issues/1746#issuecomment-499428621.

With the better versioning, we can use the current duplication error and add an additional check during the webpack compilation.

OTOH this won't fix the main topic of this issue - Adding plugins to CKEditor 5 builds 😅 But it will help with installing outdated packages etc.

Reinmar commented 5 years ago

DUP reported in https://github.com/ckeditor/ckeditor5/issues/1774

And my answere there:

The short answer for what are the reasons why adding plugins to CKEditor 5 cannot be achieved by a simple concatenation is that our experience from the past (CKEditor 3-4 allows that). There are significant tradeoffs when you try to enable that – you either build your own module system (and sometimes a bundler) with lost of hacks under the hood (which sometimes backfire at you) or you create a code in a monolithic way, with little code reuse and finally make your builds extremely bloated (containing a lot of unnecessary code).

I haven't yet seen an elegant solution to that other than keeping your code modular and involving a bundler (like webpack).

That said, we faced the "can't easily add plugin to a build" issue ourselves and we know tgat it affects a part of our community too. Not everyone, because in many projects webpack is a standard, but still some significant part. The problem here turns out to be not the legacy envs where people don't yet use webpack, but rather frameworks envs because they come with an already really complicated webpack configs and developers are not happy to change those.

Therefore, we'll be looking for solutions in #667 very soon. This is a big priority for us and there's a chance that we'll be doing significant changes to workaround those issues. We still want to stay as modular as possible, but perhaps we can do something to reduce the pain significantly. That said, it will take some time for us to implement the solution.

Reinmar commented 5 years ago

Related ticket: https://github.com/ckeditor/ckeditor5/issues/649.

kromit commented 5 years ago

I just wanted to let you know that this issue was the main reason for us for not using this editor. Not a single other widget (we use about 20), including all of your direct competitors, requires me to tamper with my webpack and in case of react event to eject the whole project. Are you guys serious? Its almost like "Oh, I see you want to buy this new pc-display. Great! But if you want to change the resolution, you have to rewire the whole building". This is ridiculous in the sense of increasing cost and decreasing stability. This issue is actively pushing people towards your competitors and I am not talking about free to use solutions.

ma2ciek commented 5 years ago

In my opinion:

We'll never move to the point where all plugins/features will be available in the editor interface as we'd have to import every single feature in the editor core and it would make the editor weight gigabytes.

CKEditor 5 builds + docs focusing on them (like Installation, Installing plugins) require to rebuild the editor whenever a new plugin is added, as the code is exported as a "library" by webpack. The https://github.com/cksource/cloud-features/issues/3009 should help here if someone without any JS-env knowledge would want to have a working editor with some set of plugins but this shouldn't be the primary way of building the editor.

On the other hand devs that want to install and keep source code (like in 90% of SPAs) require building-from-source approach which is hidden in docs while IMO this is the best way to add the CKEditor 5 to the application without worrying that with every release/change in the plugin set there is a need to rebuild the editor.

And here we have the versioning issue, which should be fixed as soon as possible to make the above approach 100% stable and the problem with building Angular together with CKEditor 5, which is maybe doable now as I tested it almost a year ago.

kgrosvenor commented 5 years ago

I am also having the exact same issue, and the plugins im importing havent been imported yet either? I've followed the docs as it says an no results. Why is this issue closed may i ask?

ma2ciek commented 5 years ago

Why is this issue closed may i ask?

This issue is open.

glennverschooren commented 4 years ago

Hi,

Is it possible to create a custom plugin and load it dynamically into the page by using a script tag? I want to wrap the bundle into a function so that I can manually set the dependencies when invoking the function. This way I can ensure that @ckeditor/ckeditor5-core and @ckeditor/ckeditor5-engine are only loaded once.

The only problem that I can think of is the fact that there is no entry file for @ckeditor/ckeditor5-core and @ckeditor/ckeditor5-engine. And because of this, it is not possible to import all features at once. import * as ckeditorEngine from "@ckeditor/ckeditor5-engine''

// Wrap the plugin with a custom webpack loader
window.MODULE_LOADER.addModuleSource('custom-plugin', (require, exports) => {
     return (Plugin code) 
});

class ModuleLoader {
    private moduleSources: {[key: string]: any} = {};

    public addModuleSource(name: string, source: any) {
        this.moduleSources[name] = source;
    }

    public getModuleSource(name: string): any {
        return this.moduleSources[name];
    }

    public loadModule(path: string, moduleName: string, deps: {[key: string]: any}): Promise<any> {
        return new Promise((resolve, reject) => {
            get(path, () => {
                this.compileSource(this.moduleSources[moduleName], deps).then((result) => {
                    resolve(result);
                }, reject);
            });
        });
    }

    private compileSource(source: any, deps: {[key: string]: any}): Promise<any> {
        const modules: {[key: string]: any} = {
            ...deps,
        };

        const require = (module: string) => modules[module];

        try {
            const exports = {};
            const result = source(require, exports);

            return Promise.resolve({
                result,
                exports,
                require
            });
        }
        catch (e) {
            return Promise.reject(e);
        }
    }
}

const deps = {
    '@ckeditor/ckeditor5-core': { ...exports },
    '@ckeditor/ckeditor5-engine': { ...exports },
};

moduleLoader.loadModule('custom-plugin', deps).then(() => {
    // M
});
Exitare commented 4 years ago

Any updates on this one?

ma2ciek commented 4 years ago

I'm afraid that adding plugins to CKEditor 5 builds is and will be impossible because of many architectural blockers such as:

For any advanced usage I'd recommend building the editor from source. This way:

On the other hand if you're not familiar with the NPM ecosystem but you want to create quickly a PoC of CKEditor 5 editor containing given set of features I'd recommend using the online builder tool.

Reinmar commented 3 years ago

Worth also including  #7981 in the "nice-to-have" requirements.

Reinmar commented 3 years ago

Another interesting idea: #8184.

jodator commented 3 years ago

Let's revive this issue and breath a new life into the matter. :zombie_man:

Key drivers to have this implemented:

  1. Ease of use by external developers.
    • In other words, it should be enable-able just by adding <script> tags of our special build and a bundled (or not) external feature.
  2. We should not lose tree-shaking ability in our builds but we're fine with bigger builds that allow adding plugins.
    • However, using the custom build, in that case, is OK.
  3. Base editor API should be de-duplicated in such builds.
    • At least enabled once - we can live with double code in that case.
  4. Public API should enable most of the integrations possible.
    • Only base editor API is needed, other things should work on top of that.
  5. CSS should work and not be duplicated.

We have talked about: