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

Cannot combine imports from ckeditor5 with deep imports #17575

Closed
bendemboski opened this issue Dec 3, 2024 · 8 comments · Fixed by #17597
Closed

Cannot combine imports from ckeditor5 with deep imports #17575

bendemboski opened this issue Dec 3, 2024 · 8 comments · Fixed by #17597
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@bendemboski
Copy link
Contributor

Consider the following code:

import { EditorUIView, type Locale, type EditingView } from 'ckeditor5';
import EditableUIView from '@ckeditor/ckeditor5-ui/dist/editableui/editableuiview.js';

export default class MyEditorUIView extends EditorUIView {
  readonly editable: EditableUIView;

  constructor(
    locale: Locale,
    editingView: EditingView,
    editableElement: HTMLElement,
  ) {
    super(locale);

    this.editable = new EditableUIView(locale, editingView, editableElement);
  }
}

As-is (running against CKEditor 43.3.1), the code produces this type error:

src/base-editor/editoruiview.ts(5,12): error TS2416: Property 'editable' in type 'MyEditorUIView' is not assignable to the same property in base type 'EditorUIView'.
  Type 'import("/project/node_modules/@ckeditor/ckeditor5-ui/dist/editableui/editableuiview").default' is not assignable to type 'import("/project/node_modules/@ckeditor/ckeditor5-ui/src/editableui/editableuiview").default'.
    Property '_editingView' is protected but type 'EditableUIView' is not a class derived from 'EditableUIView'.

This is because ckeditor5 resolves to ckeditor5/dist/index.d.ts, which re-exports the contents of @ckeditor/ckeditor5-ui, which resolves to @ckeditor/ckeditor5-ui/src/index.d.ts (note that we've "hopped over" from dist/ directories to src/ directories). Since @ckeditor/ckeditor5-ui/dist/index.d.ts is a copy of @ckeditor/ckeditor5-ui/src/index.d.ts (rather than re-exporting it), they export different symbols that share the same name EditorUIView, resulting in the error above.

The type error can be fixed by changing line 2 to import EditableUIView from '@ckeditor/ckeditor5-ui/src/editableui/editableuiview'; (replacing dist with src). However, this causes module duplication because while the types in ckeditor5 re-export @ckeditor/ckeditor5-ui/src, the Javascript re-exports @ckeditor/ckeditor5-ui/dist/index.js. So now the types agree, but the bundled application will include both @ckeditor/ckeditor5-ui/dist/index.js (from the import via ckeditor) and @ckeditor/ckeditor5-ui/src/editableui/editableuiview.js (via the deep import), and now at runtime there will be two copies of the EditableUIView class, inflating the bundle and potentially messing up instanceof checks and the like.

I just used @ckeditor/ckeditor5-ui as an example -- the same issue exists with all the @ckeditor/ckeditor5-* packages.

The fundamental problem here is that dist is not isolated from src, and anytime the module graph "hops over" from dist to src, there will likely be a conflict that results in either type errors or module duplication.

This also runs deeper than just ckeditor5/dist/index.d.ts -- there are many .d.ts files in dist directories of @ckeditor/ckeditor5-* packages that import from the module root of other @ckeditor/ckeditor5-* packages, which resolves to src, for example the import from @ckeditor/ckeditor5-utils here, and there are many many other examples. So it's simply not safe to consume files from dist/ folders because they might pull in files from src/ folders that conflict with the corresponding dist/ file imported elsewhere in the module graph.

❓ Possible solution

The most straightforward solution would probably be to change the @ckeditor/ckeditor5-* packages' main and types entries in package.json to point to dist rather than src, but this would be a change to the module layout that might raise compatibility issues.

The other solution would be to ensure that modules in dist/ always import from @ckeditor/ckeditor5-*/dist/* rather than from the @ckeditor/ckeditor5-* module roots.

📃 Other notes

This makes importing from the ckeditor5 module root, as well as consuming modules anywhere in dist/, un-tenable for my application at the moment. A workaround is to import from ckeditor5/src/index.js rather than ckeditor5, so I'm only consuming from src/, but based on the release notes, my impression is that CKEditor is trying to move consumers from src/ to dist/. Unfortunately, that's not possible for me right now.


If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@bendemboski bendemboski added the type:bug This issue reports a buggy (incorrect) behavior. label Dec 3, 2024
@filipsobol
Copy link
Member

The dist folder used for new installation methods has a different structure from the src folder. The src folder has the same structure as the source code, so import like this is possible:

// ✔️
import {} from '@ckeditor/ckeditor5-ui/src/editableui/editableuiview.js';
//                                     ^^^

However, the dist folder only contains a single index.js which includes everything from a given package. That's why the following import is not valid:

// ✖️
import {} from '@ckeditor/ckeditor5-ui/dist/editableui/editableuiview.js';
//                                     ^^^^

In new installation methods, only the following imports are allowed:

import {} from 'ckeditor5';
import {} from '@ckeditor/ckeditor5-ui/dist/index.js';

Unfortunately, it's not possible to import EditableUIView this way because we currently don't export this class. We can add this missing export, but I'd appreciate it if you could tell us why you need it because we try to limit the number of exports.

@Witoso
Copy link
Member

Witoso commented Dec 3, 2024

Also, please check #17289 for even more details.

src will be deprecated in the future, I will do some announcing this month as we will prolong the period we proposed here. I assume src will be with us for the whole 2025.

@bendemboski
Copy link
Contributor Author

@filipsobol @Witoso thanks for your replies. I guess the use of a symbol that isn't exported by ckeditor5 is causing some confusion. Let me try again.

The documentation suggests deep imports from @ckeditor/ckeditor5-*/dist/ as a way to optimize the build size. This issue is pointing out that this can't be safely mixed with imports from ckeditor5 or the types can come into conflict.

Here's perhaps a cleaner example:

import { ClassicEditor, Paragraph } from 'ckeditor5';
import type { Plugin } from '@ckeditor/ckeditor5-core/dist/index.js';

const editorConfig = {
  plugins: [Paragraph],
  licenseKey: 'GPL',
};

function logEnabled(plugin: Plugin) {
  console.log('Plugin:', plugin.isEnabled);
}

ClassicEditor.create(document.querySelector('#editor') as HTMLElement, editorConfig).then((editor) => {
  let plugin = editor.plugins.get('Paragraph');
  logEnabled(plugin);
});

This produces a type error

src/main.ts:15:13 - error TS2345: Argument of type 'Paragraph' is not assignable to parameter of type 'Plugin'.
  The types of 'editor.accessibility' are incompatible between these types.
    Type 'import("/project/node_modules/@ckeditor/ckeditor5-core/src/accessibility").default' is not assignable to type 'import("/project/node_modules/@ckeditor/ckeditor5-core/dist/accessibility").default'.
      Types have separate declarations of a private property '_editor'.

15  logEnabled(plugin);
               ~~~~~~

because ckeditor5/dist/index.d.ts re-exports from @ckeditor/ckeditor5-core/src/index.d.ts rather than @ckeditor/ckeditor5-core/dist/index.d.ts.

Is this intentional? This seems like a bug.

@filipsobol
Copy link
Member

I see the problem now, and this is not intentional. Thank you for bringing this to our attention. I'll look into it.

As a side note, if you want to optimize the build size, all imports should be from the @ckeditor/ckeditor5-*/dist/ packages. Even a single import from ckeditor5 will make the whole effort pointless. I recently had an idea on how to fix this, so that imports from @ckeditor/ckeditor5-*/dist/ are not unnecessary, which we may implement in Q1.

@filipsobol
Copy link
Member

@bendemboski Do you use the EditableUIView class? I'm not sure if we should export it, or if you only used it as an example 🙂

@bendemboski
Copy link
Contributor Author

bendemboski commented Dec 4, 2024

@filipsobol great, glad we're on the same page, and sorry my initial explanation was kinda convoluted.

Curious -- what is it about imports through ckeditor5 that prevents the bundle size optimizations, and what kind of extra code gets included compared to importing from @ckeditor/ckeditor5-*/dist?

As far as EditableUIView, I'm in the process of finishing up the upgrade to CKEditor 44 (our app is pretty massive and very heavily uses CKEditor, so it's a significant effort), and then I'm going to chase down all the symbols we import that are not exported via the "new API" and figure out which ones I think we really need, and then I was planning to file a single issue describing them all (and we could split out into multiple issues from there if that makes sense).

So standby, and hopefully in the next few business days I'll open a new issue for discussion about all of the un-exported symbols we use 👍

@filipsobol
Copy link
Member

[...] what kind of extra code gets included compared to importing from @ckeditor/ckeditor5-*/dist?

It's mostly code from external dependencies used in the @ckeditor/ckeditor5-markdown-gfm plugin, which aren't tree-shakeable. If you don't use this plugin, you'll get up to 45 KiB (before compression) of extra code that you won't use.

You can read more about it here: #16395

@bendemboski
Copy link
Contributor Author

@filipsobol I just filed #17644 that describes why we need EditableUIView (and a whole bunch of other symbols that aren't exported in the new installation method as well)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants