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

Add ELSint rule to check inter-packages imports #8581

Closed
jodator opened this issue Dec 3, 2020 · 5 comments · Fixed by ckeditor/eslint-plugin-ckeditor5-rules#2
Closed

Add ELSint rule to check inter-packages imports #8581

jodator opened this issue Dec 3, 2020 · 5 comments · Fixed by ckeditor/eslint-plugin-ckeditor5-rules#2
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:extending-builds 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 Dec 3, 2020

📝 Provide a description of the new feature

Follow-up for the #8395. We're going to change how we import things from other packages.

This ESLint rule should check if the imports from other packages are pointing to the package entry file instead of direct import.

In other words, depending on how the #8578 is resolved the imports should point to entry file:

import { Plugin } from '@ckeditor/ckeditor5-core/core';
// or (see #8578)
import { Plugin } from 'ckeditor/core';

The wrong import would be:

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

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). domain:dx This issue reports a developer experience problem or possible improvement. squad:dx labels Dec 3, 2020
@jodator jodator added this to the backlog milestone Dec 3, 2020
@Reinmar Reinmar modified the milestones: backlog, nice-to-have Dec 15, 2020
@Reinmar Reinmar modified the milestones: nice-to-have, iteration 39 Jan 7, 2021
@Reinmar
Copy link
Member

Reinmar commented Jan 7, 2021

Verify that the only thing that a package has in dependencies is ckeditor5 (from the set of pkgs maintained by us). Second rule – only import from the current package or ckeditor5 (also, only applies to the set of our packages). Let's have ESLint rules for that (#8581). Exception: the ckeditor5 package.

@Reinmar
Copy link
Member

Reinmar commented Jan 7, 2021

TODO: Update remaining incorrect imports (also in CF and internal).

@pomek
Copy link
Member

pomek commented Jan 8, 2021

In the Title plugin we need to refactor the code a bit:

-import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
-import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import { Plugin } from 'ckeditor5/src/core';
 
-import ViewDocumentFragment from '@ckeditor/ckeditor5-engine/src/view/documentfragment';
-import ViewDowncastWriter from '@ckeditor/ckeditor5-engine/src/view/downcastwriter';
-import first from '@ckeditor/ckeditor5-utils/src/first';
+import { first } from 'ckeditor5/src/utils';
 import {
+	UpcastWriter,
 	needsPlaceholder,
 	showPlaceholder,
 	hidePlaceholder,
 	enablePlaceholder
-} from '@ckeditor/ckeditor5-engine/src/view/placeholder';
+} from 'ckeditor5/src/engine';
@@ -44,7 +42,7 @@ export default class Title extends Plugin {
 	 * @inheritDoc
 	 */
 	static get requires() {
-		return [ Paragraph ];
+		return [ 'Paragraph' ];
 	}
 	/**
@@ -149,10 +147,11 @@ export default class Title extends Plugin {
 		const data = editor.data;
 		const model = editor.model;
 		const root = editor.model.document.getRoot();
-		const viewWriter = new ViewDowncastWriter( editor.editing.view.document );
+		const view = editor.editing.view;
+		const viewWriter = view.change( viewWriter => viewWriter );
 
 		const rootRange = model.createRangeIn( root );
-		const viewDocumentFragment = new ViewDocumentFragment( editor.editing.view.document );
+		const viewDocumentFragment = new UpcastWriter( view.document ).createDocumentFragment();
 
 		data.downcastDispatcher.conversionApi.options = options;

Are we fine with the changes above?

@Reinmar
Copy link
Member

Reinmar commented Jan 8, 2021

As for the viewWriter... This is a hacky code in that plugin, so there will be no prefect solution. I'd go with adding ViewDowncastWriter as additional export in ckeditor5-engine. Let's make this class accessible because I remember that I also had to use it once somewhere.

@Reinmar
Copy link
Member

Reinmar commented Jan 8, 2021

@pomek pointed out that this will mean using two different writers in one piece of code. I thought at first that these are two different methods.

This piece of code is copied directly from DataController#get(). We could think about exposing somehow a piece of that logic and make it reusable, but this is for the future. For now, I think it's reasonable to add createDocumentFragment() to DowncastWriter. It's apparently needed when working in both directions (up and down).

Reinmar added a commit to ckeditor/eslint-plugin-ckeditor5-rules that referenced this issue Jan 12, 2021
Feature: Added a rule that disallows direct imports of packages that belong to CKEditor 5 DLL. Closes ckeditor/ckeditor5#8581.
Reinmar added a commit that referenced this issue Jan 12, 2021
Internal: Updated import paths. See #8581.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:extending-builds 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.

3 participants