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

Implement DLLs for CKEditor 5 features #8517

Closed
jodator opened this issue Nov 24, 2020 · 10 comments · Fixed by #8564
Closed

Implement DLLs for CKEditor 5 features #8517

jodator opened this issue Nov 24, 2020 · 10 comments · Fixed by #8564
Assignees
Labels
domain:extending-builds package:core package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@jodator
Copy link
Contributor

jodator commented Nov 24, 2020

📝 Provide a description of the new feature

As the DLL spike research proved the approach, we will implement the DLLs for the CKEditor 5 as multiple DLLs.

During F2F talks with @Reinmar & @pomek, we've identified that implementing DLLs might be created on multiple levels to provide reuse of core CKEditor packages and to minimize, if possible, the number of <script> tags needed to insert into the document. To do that, we would choose among below 3 options:

1st option:

  • base DLL (core + utils + engine + ui + widget? + essentials? + paragraph?): ckeditor5
  • creator DLL (classic, balloon, ...): @ckeditor/ckeditor5-editor-*
  • other package DLLs (image, basic-styles, ...): @ckeditor/ckeditor5-*

2nd option:

  • base DLL (core + utils + engine + UI): ckeditor5
  • creator DLL (classic, balloon, ... + + widget + essentials + paragraph + basic-styles + image, etc.): @ckeditor/ckeditor5-editor-*
  • other package DLLs (image, basic-styles, ...): @ckeditor/ckeditor5-*

3rd option:

  • base DLL
  • build DLL
  • other package DLLs, not included in build DLL

Potential issues:

  • Angular, React, Vue: integrations must not import anything.
  • Collaboration features.
  • How to import from base DLL as sane imports (ideally the same as we use today). We should not alter current imports to not break current builds (tests, manual tests, etc.).

Additionally, I see option 0: every package is a DLL.


If you'd like to see this feature implemented, add a 👍 reaction to this post.

@jodator jodator added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:core package:engine squad:dx domain:extending-builds labels Nov 24, 2020
@jodator jodator added this to the iteration 38 milestone Nov 24, 2020
@jodator jodator self-assigned this Nov 24, 2020
@jodator
Copy link
Contributor Author

jodator commented Nov 24, 2020

OH, I've just stumbled on first obstacle, that might be hard to overcome in a granular approach. It looks like intermediate DLL is not possible. So it's either a DLL or a DLL consumer.

If I cannot overcome this issue, I'll probably make a shortlist of CKEditor plugins that need to be in every build.

@jodator
Copy link
Contributor Author

jodator commented Nov 26, 2020

At this stage, using top-level webpack's plugins for DLLs I don't see that we could implement options 0-3.

Problem 1: No DLL in other DLL

The reason for this is that in one webpack config there can be either DLLPlugin or DllReferencePlugin. This means that you can't create (at least using top-level plugins) a DLL that consumes other DLL. Well, at least to my knowledge and understanding of things.

From this point I can see that we could:

  1. Explore a possibility to use lower-level DLL plugins (or writing our own DLL plugin that would fill a missing gap for a DLL that can also consume other DLLs.
  2. Write a totally custom plugin that would produce the proper reference plugin entries.

ATM, I'm leaning towards one super DLL with (more on cross-package dependencies in #8402):

  • utils
  • engineutils
  • coreutils, engine
  • uiutils, core
  • *-editorutils, engine, core, ui (optional)
  • essentials
    • clipboardutils, engine, core
    • enterutils, engine, core
    • select-allutils, core, ui
    • typingutils, engine, core
    • undoengine, core, ui
  • widgetutils, engine, core, ui, typing
  • paragraphutils, core, ui

Optionally, we should investigate whether cross-packages imports makes sense for:

  • image
  • upload

All other packages might be created as a DLL consumer and be exposed as UMD build (or just global).

Problem 2: imports in DLL consumer

The other thing that I've discovered is (a similar issue was raised by @pomek) that writing code imports for DLL is different than for our own code. This is probably easier to solve with a custom plugin.

The thing is that it that I would like to remain imports in sync with the paths in packages (might require index.js anyway). So in other words one DLL should act as a multiple entry for all it's bundled packages.

For instance, if our end-plugin would have imports:

import Plugin from '@ckeditor/ckeditor5-core';
import EmitterMixin from '@ckeditor/ckeditor5-utils';

It should be picked up by webpack in DLL consumer mode as (Those are already available in my DLL POC):

import Plugin from '@ckeditor/ckeditor5-dll/core';
import EmitterMixin from '@ckeditor/ckeditor5-dll/utils';

It looks like a simple string replacement before the DLLReferencePlugin would do its magic.

I've tried to:

  • use NormalModuleReplacementPlugin - without luck. The replacement resolution is done after the DLL reference plugin.
  • use resolve.alias configuration. Similarly, without luck. The resolve is done even further in the compile stage (AFAIR it was used by module loader) so also too late.

@jodator
Copy link
Contributor Author

jodator commented Dec 2, 2020

Recent findings on how the DLL builds work:

  • The DLL build (vendor) requires to provide all the exports needed by DLL consumers.
  • As long as the DLL exported modules remain unchanged - the DLL vendor might be rebuild without requiring the DLL consumers to be rebuild.
    • This is because the imports in DLL may operate on named module ids
    • The above configuration will yield the same module id on each build.
  • Using one base, DLL build does not produce "duplicated packages error" as long as we rename imports to be consistent with the index.js export.

This proves the POC but still requires work to have a sane development environment. This is because I couldn't find a way to use DLLReferencePlugin as below:

import { Plugin } from '@ckeditor/ckeditor5-core/index'        // or without index
import { ButtonView } from '@ckeditor/ckeditor5-ui/index'

ATM I can use DLLReferencePlugin as:

import { Plugin } from '@ckeditor/ckeditor5-core/core'
import { ButtonView } from '@ckeditor/ckeditor5-ui/ui'

because those are just the same as (notice the @ckeditor/ckeditor5-dll package name, which can be anything):

import { Plugin } from '@ckeditor/ckeditor5-dll/core'
import { ButtonView } from '@ckeditor/ckeditor5-dll/ui'

@jodator
Copy link
Contributor Author

jodator commented Dec 2, 2020

Thanks to the @pomek's work on the CKEdtior 5 global I've added a Basic Styles feature as a DLL consumer. 

What I was able to achieve is this:

<!-- This adds the CKEditor5_DLL in the global scope, so it is possible to reference its contents -->
<script src="../build/ckeditor5-dll.js"></script>

<!-- This is a stripped-down ClassicEditor with Essentials plugins loaded from a DLL, exposed as ClassicEditor -->
<script src="../../ckeditor5-dll-classic/build/dll-classic.js"></script>

<!-- Editor features as DLL consumer builds exposed as CKEditor5[FeatureName] -->
<script src="../../ckeditor5-basic-styles/build/basic-styles.js"></script>

<!-- Finally, this is a end-user plugin that uses DLL -->
<script src="../build/dll-consumer-plugin.js"></script>

<script>
	// Import Bold, Italic from the CKEditor 5 global.
	const { Bold, Italic } = CKEditor5.BasicStyles;

	const config = {
		extraPlugins: [
			Bold,
			Italic,
			DLLConsumerPlugin // exposed by the dll-consumer-plugin.js
		],
		toolbar: [
			'bold',
			'italic',
			'|',
			'a-button',
			'|',
			'undo',
			'redo'
		]
	};

	// ClassicEditor is exposed by the ckeditor5-dll-classic:
	ClassicEditor.create( document.querySelector( '#editor' ), config )
		.then( editor => {
			window.editor = editor;
		} );
</script>

Despite some nuances regarding the need to rewrite inter-package imports in DLL consumer plugins, this looks kinda awesome.

Right now a manual work will be needed to align all the features to DLL approach as additional boilerplate code needs to be written. However, after the initial work, we should not modify them too often.

Edit: This is mostly due to cross-packages imports: #8402 (comment).

@jodator
Copy link
Contributor Author

jodator commented Dec 2, 2020

And two editor DLL builds (Classic + Inline) on the same page:

<!-- This adds the CKEditor5_DLL in the global scope, so it is possible to reference its contents -->
<script src="../build/ckeditor5-dll.js"></script>

<!-- Those are stripped-down editors with Essentials plugins loaded from a DLL, exposed on CKEditor global -->
<script src="../../ckeditor5-dll-classic/build/dll-classic.js"></script>
<script src="../../ckeditor5-dll-inline/build/dll-inline.js"></script>

<!-- Editor features as DLL consumer builds exposed as CKEditor5[FeatureName] -->
<script src="../../ckeditor5-basic-styles/build/basic-styles.js"></script>

<!-- Finally, this is a end-user plugin that uses DLL -->
<script src="../build/dll-consumer-plugin.js"></script>

<script>
    // Import Bold, Italic from the CKEditor 5 global.
    const { Bold, Italic } = CKEditor5.BasicStyles;
    const { ClassicEditor, InlineEditor } = CKEditor5;

    const config = {
        extraPlugins: [
            Bold,
            Italic,
            DLLConsumerPlugin // exposed by the dll-consumer-plugin.js
        ],
        toolbar: [
            'bold',
            'italic',
            '|',
            'a-button',
            '|',
            'undo',
            'redo'
        ]
    };

    // ClassicEditor is exposed by the ckeditor5-dll-classic:
    ClassicEditor.create( document.querySelector( '#editor-classic' ), config )
        .then( editor => {
            window.editor = editor;

            CKEditorInspector.attach( { 'dll-classic': editor } );
        } );

    // InlineEditor is exposed by the ckeditor5-dll-inline:
    InlineEditor.create( document.querySelector( '#editor-inline' ), config )
        .then( editor => {
            window.editor = editor;

            CKEditorInspector.attach( { 'dll-inline': editor } );
        } );
</script>

@jodator
Copy link
Contributor Author

jodator commented Dec 3, 2020

DLL system plan of action

Below are most of the changes required to create CKEditor 5 DLL system. There are 2 components of this:

  1. Base DLL package with webpack.DLLPlugin configuration in webpack.config.
  2. A DLL-consumer package, that automatically adds its public-public API to the CKEditor5 global.

Packages to include in base DLL

This set of packages will shape the size and features available. Either those are required packages by the editor or are functionalities that will end up in 80% of the editors.

  • Base packages
    • ckeditor5-utils
    • ckeditor5-core
    • ckeditor5-engine
    • ckeditor5-ui
  • Must-have packages (Essentials)
    • ckeditor5-enter
    • ckeditor5-paragraph
    • ckeditor5-select-all
    • ckeditor5-typing
    • ckeditor5-undo
    • ckeditor5-widget
  • Base editors classes
    • ckeditor5-editor-balloon
    • ckeditor5-editor-classic
    • ckeditor5-editor-decoupled
    • ckeditor5-editor-inline
  • Potentially required in DLL
    • ckeditor-cloud-services-core
    • ckeditor5-upload

The public-public API, defined as anything that must be imported by other packages, should be defined in main .js file of the package.

The base editor packages are in the DLL build because those are intermediate packages. Those are using DLL packages and expose themselves as a base editor classes. The DLL system does not allow it explicitly so those must land in base DLL to reduce code duplication.

The editor builds are provided as other plugins and will be exposed on CKEditor5 global (e.g. CKEditor5.ClassicEditor). Those are thin wrappers and could be included in the DLL if we figure out how to expose CKEditor5 global from the DLL itself.

New packages

The outcome of this will be new packages, although maybe we could expose DLL builds from the build packages to reduce overhead.

  • ckeditor5-dll
  • ckeditor5-dll-balloon
  • ckeditor5-dll-balloon-block
  • ckeditor5-dll-classic
  • ckeditor5-dll-decoupled-document
  • ckeditor5-dll-inline

Packages that are used during the build

This package(s) are used by DLL build and changes in them are not needed ATM.

  • ckeditor5-theme-lark

Packages to be provided as DLL-consumer

  • ckeditor5-adapter-ckfinder
  • ckeditor5-alignment
  • ckeditor5-autoformat
  • ckeditor5-autosave
  • ckeditor5-basic-styles
  • ckeditor5-block-quote
  • ckeditor5-ckfinder
  • ckeditor5-clipboard
  • ckeditor5-cloud-services
  • ckeditor5-code-block
  • ckeditor5-easy-image
  • ckeditor5-font
  • ckeditor5-heading
  • ckeditor5-highlight
  • ckeditor5-horizontal-line
  • ckeditor5-html-embed
  • ckeditor5-image
  • ckeditor5-indent
  • ckeditor5-link
  • ckeditor5-list
  • ckeditor5-markdown-gfm
  • ckeditor5-media-embed
  • ckeditor5-mention
  • ckeditor5-page-break
  • ckeditor5-paste-from-office
  • ckeditor5-remove-format
  • ckeditor5-restricted-editing
  • ckeditor5-special-characters
  • ckeditor5-table
  • ckeditor5-watchdog
  • ckeditor5-word-count

Alternatives to DLL (no action)

I don't see a need for essentials ATM as it is a shortcut plugin for the standard builds.

  • ckeditor5-essentials (?)
  • ckeditor5-build-balloon
  • ckeditor5-build-balloon-block
  • ckeditor5-build-classic
  • ckeditor5-build-decoupled-document
  • ckeditor5-build-inline

Open questions:

  1. How to expose icons to be reusable and if we should care at all. ATM those are duplicated. (Follow-up created: Reusable icons from DLL build #8574).
  2. The import path is imperfect ATM. (Check if DLL can be exposed in ckeditor5 main repository #8578)
  3. I've used ckeditor5-dll as the entry point for building main DLL. It could be moved to the ckeditor5 probably. (Check if DLL can be exposed in ckeditor5 main repository #8578)
  4. CKEditor 5 DLL is exposed as CKEditor5_DLL, but could be anything else. Including, but not limited to, CKEditor5.DLL. (follow-up: Decide on DLL global location #8575)
  5. Should we expose Essentials as a DLL-consumer? It's in the base editors anyway, so I don't see a need for this. (follow-up: Validate a need for ckeditor5-essentials to be exposed as DLL-consumer plugin #8576).

@jodator
Copy link
Contributor Author

jodator commented Dec 3, 2020

Ad 2.: I've tried to use main entry in package.json but the scope configuration must preeceed the imported module. Right now I can see that we could use (together with exposing those in main ckeditor5 package):

import { Plugin } from 'ckeditor5/core';

@jodator
Copy link
Contributor Author

jodator commented Dec 3, 2020

How to debug DLL imports (if everytihng is OK) in case you get "duplicated" module errors.

  1. Enable full webpack output using stats: 'verbose' preset.
  2. Look for the first ../ckeditor5-package/src/... entry (not theme nor icon).
  3. Look by what it was imported after the "harmony side effect evaluation".
  4. Rebuild & see for other output errors.

Example output:

[../ckeditor5-core/theme/icons/object-size-small.svg] 766 bytes {path} [depth 3] [built]
     [exports: default]
     harmony side effect evaluation @ckeditor/ckeditor5-core/theme/icons/object-size-small.svg [./src/imageresize/imageresizebuttons.js] 16:0-83
     harmony import specifier @ckeditor/ckeditor5-core/theme/icons/object-size-small.svg [./src/imageresize/imageresizebuttons.js] 22:8-17
 [../ckeditor5-engine/src/view/placeholder.js] 9.13 KiB {path} [depth 4] [built]
     [exports: enablePlaceholder, disablePlaceholder, showPlaceholder, hidePlaceholder, needsPlaceholder]
     harmony side effect evaluation @ckeditor/ckeditor5-engine/src/view/placeholder [./src/imagecaption/utils.js] 10:0-84
     harmony import specifier @ckeditor/ckeditor5-engine/src/view/placeholder [./src/imagecaption/utils.js] 25:2-19
 [../ckeditor5-engine/theme/placeholder.css] 614 bytes {path} [depth 5] [built]

The culprit in Image files was a left over:

import { enablePlaceholder } from '@ckeditor/ckeditor5-engine/src/view/placeholder';
import { toWidgetEditable } from '@ckeditor/ckeditor5-widget/src/utils';

Which should be:

import { enablePlaceholder } from '@ckeditor/ckeditor5-engine/engine';
import { toWidgetEditable } from '@ckeditor/ckeditor5-widget/widget';

A proper entry should be:

[@ckeditor/ckeditor5-engine/engine.js] delegated ./engine.js from dll-reference CKEditor5_DLL 42 bytes {path} [depth 2] [built]
     [exports: enablePlaceholder, disablePlaceholder, showPlaceholder, hidePlaceholder, needsPlaceholder, EditingController, DataController, Conversion, LiveRange, LivePosition, Model, Observer, UpcastWriter, StylesProcessor]

@jodator
Copy link
Contributor Author

jodator commented Dec 4, 2020

The initial DLL build is done. We have a working DLL sample with 4 different editor "builds" and a few plugins: basic styles, image and html embed. 🎉 .

jodator added a commit that referenced this issue Dec 4, 2020
Feature: Introduce the CKEditor 5 DLL build. Closes #8517.
@jodator
Copy link
Contributor Author

jodator commented Dec 4, 2020

Closed by 577add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:extending-builds package:core package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants