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

Allow adding plugins to a CKEditor 5 build #667

Closed
pjasiun opened this issue Nov 15, 2017 · 51 comments
Closed

Allow adding plugins to a CKEditor 5 build #667

pjasiun opened this issue Nov 15, 2017 · 51 comments
Labels
status:discussion type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@pjasiun
Copy link

pjasiun commented Nov 15, 2017

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.

@Reinmar Reinmar changed the title Adding plugins to the CKEditor 5 build Adding plugins to a CKEditor 5 build Nov 15, 2017
@pjasiun
Copy link
Author

pjasiun commented Nov 15, 2017

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
Copy link
Member

Reinmar commented Nov 15, 2017

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 Reinmar added status:discussion type:improvement This issue reports a possible enhancement of an existing feature. labels Nov 15, 2017
@Reinmar
Copy link
Member

Reinmar commented Mar 19, 2018

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.

@Reinmar Reinmar added this to the iteration 15 milestone Mar 19, 2018
@oleq
Copy link
Member

oleq commented Mar 19, 2018

This asks for an entry in FAQ.

@Reinmar
Copy link
Member

Reinmar commented Mar 19, 2018

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
Copy link
Member

Reinmar commented Mar 29, 2018

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
Copy link
Member

Reinmar commented Apr 12, 2018

Another issue caused, most likely, by package duplication: #957

There are a couple more things I'm considering:

  • How to avoid importing Plugin? Could plugins be bare classes and just be extended when editor initializes them? How messy will it be?
  • How to avoid importing Command? Can it be the same solution as for plugins?
  • Which UI components could have creators built into the editor.ui? Would we be able to define a reasonable number of them? Button, dropdown(s)... but what about all the floating stuff? How many of these things are always (even in the simplest editor setups) used anyway?

@Reinmar
Copy link
Member

Reinmar commented Apr 12, 2018

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

  • this package will not have breaking changes too often,
  • it somehow specifies with which version of the whole framework the feature is compatible with.

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

@Reinmar
Copy link
Member

Reinmar commented Apr 12, 2018

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
Copy link
Contributor

ma2ciek commented Apr 12, 2018

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
Copy link
Member

Reinmar commented Apr 12, 2018

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
Copy link
Contributor

ma2ciek commented Apr 12, 2018

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

node_modules
  - @ckeditor/[email protected]
  - @ckeditor/ckeditor5-build-classic
     - node_modules
        @ckeditor/[email protected]

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

@Reinmar
Copy link
Member

Reinmar commented Apr 17, 2018

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 Reinmar changed the title Adding plugins to a CKEditor 5 build Allow adding plugins to a CKEditor 5 build Apr 17, 2018
@Reinmar
Copy link
Member

Reinmar commented Apr 17, 2018

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
Copy link
Author

pjasiun commented Apr 18, 2018

👍 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
Copy link
Member

Reinmar commented Apr 20, 2018

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

@Reinmar
Copy link
Member

Reinmar commented May 30, 2018

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...

@glennverschooren
Copy link

glennverschooren commented Feb 3, 2020

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
Copy link

Exitare commented May 12, 2020

Any updates on this one?

@ma2ciek
Copy link
Contributor

ma2ciek commented May 13, 2020

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

  • duplicated CSS loaded in the incorrect order,
  • duplicated CKEditor 5 singleton classes (major problem),
  • instanceof checks,
  • increased bundle size (probably ~50%) since the CKEditor 5 source code won't be able to reuse code bundled in the builds.

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

  • you won't have any duplication problem,
  • updates of CKEditor 5 dependencies will be easy,
  • you will be able to easily write your own plugins for the CKEditor 5 editor,
  • you can easily add / remove official CKEditor 5 plugins.

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
Copy link
Member

Reinmar commented Oct 5, 2020

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

@Reinmar
Copy link
Member

Reinmar commented Oct 8, 2020

Another interesting idea: #8184.

@Reinmar Reinmar modified the milestones: nice-to-have, iteration 38 Oct 26, 2020
@jodator jodator self-assigned this Oct 27, 2020
@jodator

This comment has been minimized.

@jodator
Copy link
Contributor

jodator commented Nov 2, 2020

Thanks for all the input on the matter. The majority of work to allow adding plugins to the build will be tracked in #8395. A rough summary of the above conversation is available here: #8395 (comment).

As we have to restart the work on this problem I'm closing this issue as there's no need to track this in two issues.

Please follow #8395 to track progres.

@jodator jodator closed this as completed Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:discussion type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

9 participants