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

PoC of new installation method #15331

Closed
wants to merge 9 commits into from

Conversation

filipsobol
Copy link
Member

Suggested merge commit message (convention)

Type: Message. Closes #000.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

Comment on lines -20 to -22
// To check if component is loaded more than once.
import '@ckeditor/ckeditor5-utils/src/version';

Copy link
Member Author

@filipsobol filipsobol Nov 8, 2023

Choose a reason for hiding this comment

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

I removed this side-effect import because in new build there will be no access to the /src folder, so it won't work as-is.

I'm not sure why it's here, and I couldn't find a ticket related to this commit. Version should already be checked by the utils package, so do we need this?

I also plan to add a static version property to the Plugin and Editor classes so that they can be compared to each other to specifically detect which plugins are causing the ckeditor-duplicated-modules error, so the highlighted check may not be needed.

@@ -2,3 +2,83 @@
* Copyright (c) 2003-2023, CKSource Holding sp. z o.o. All rights reserved.
* For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
*/

/* TODO: Order */
Copy link
Member Author

Choose a reason for hiding this comment

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

The order of the imports is important here. For example, with the current order, the "Powered by CKEditor" banner isn't positioned correctly if there's content after the editor.

I need to spend some more time to find the optimal order, but maybe @oleq can point out any obvious problems with this order?

Copy link
Member

Choose a reason for hiding this comment

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

The order of the imports is important here. For example, with the current order, the "Powered by CKEditor" banner isn't positioned correctly if there's content after the editor.

Can you post a screenshot of that? It should not matter because "Powered by" banner is positioned absolutely (like any other balloon in the UI) and lives far aways from the editing root element in DOM (in the "body" collection).

@@ -38,7 +38,6 @@ import {
} from '@ckeditor/ckeditor5-utils';

import '../../theme/components/dropdown/toolbardropdown.css';
import '../../theme/components/dropdown/listdropdown.css';
Copy link
Member Author

@filipsobol filipsobol Nov 8, 2023

Choose a reason for hiding this comment

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

We no longer need to import the empty CSS files, because the entire theme-lark is built as one CSS file. This increases the base CSS size by about 10kB gzipped, but we no longer need CKEditor specific plugins for webpack or Vite.

⚠️ Note that this is a breaking change, so we might need to keep these imports and empty CSS files if we want to still support old installation method. ⚠️

compilerOptions: {
declarationDir: `${ cwd }/dist/types`,
declaration: true,
declarationMap: false, // TODO
Copy link
Member Author

@filipsobol filipsobol Nov 8, 2023

Choose a reason for hiding this comment

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

This option is probably not needed, but if we have problems with VSCode or other editors navigating to .d.ts files instead of the source .ts files, then we need to enable this option during development.

terser( {
format: {
// Remove all comments except third-party licenses and the license banner from above (starting with `!`).
comments: ( node, comment ) => /@license/.test( comment.value ) && ( /^!/.test( comment.value ) || !/CKSource/.test( comment.value ) )
Copy link
Member Author

@filipsobol filipsobol Nov 8, 2023

Choose a reason for hiding this comment

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

I'm not a lawyer, but I'm guessing that we need to keep licenses of 3rd party libraries even in minified CDN builds. Is it safe to assume that these license comments will contain @license?

@Witoso
Copy link
Member

Witoso commented Nov 8, 2023

What are the reasons behind the TS bump?

@filipsobol
Copy link
Member Author

What are the reasons behind the TS bump?

I updated TS to the latest version to be able to use "moduleResolution": "bundler" added in TypeScript 5.0. It tells TypeScript to ignore lack of file extensions in relative imports, because we will bundle the source code anyway.

"moduleResolution": "bundler",

More information: https://www.typescriptlang.org/tsconfig#moduleResolution

@@ -11,7 +11,7 @@

// Importing types for this package is problematic, so it's omitted.
// @ts-ignore
import TurndownService from 'turndown';
import TurndownService from 'turndown/lib/turndown.browser.es';
Copy link
Member Author

Choose a reason for hiding this comment

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

The default turndown bundle contains require('jsdom'), so I updated the import to use the browser version instead.

@filipsobol filipsobol self-assigned this Nov 22, 2023
sourcemap: sourceMap,
banner
},
external,
Copy link
Member Author

Choose a reason for hiding this comment

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

Change to external: id => external.some( name => id.startsWith( name ) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants