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

DLL - updated imports #8783

Merged
merged 11 commits into from
Jan 12, 2021
Merged

DLL - updated imports #8783

merged 11 commits into from
Jan 12, 2021

Conversation

pomek
Copy link
Member

@pomek pomek commented Jan 8, 2021

Suggested merge commit message (convention)

Internal: Updated import paths. See #8581.


Additional information

After merging the PR, remember about adding the note below to the list of breaking changes here – #8614 (comment).

BREAKING CHANGE: The `Title` no longer load `Paragarph`. Make sure to add `Paragraph` to the editor plugins when using `Title` feature.

@pomek
Copy link
Member Author

pomek commented Jan 8, 2021

This PR requires additional eslint rules:

In order to test everything, you need to clone additional repositories. Assuming, that your current working directory is set to ckeditor workspace:

  • git clone git@github.com:ckeditor/eslint-plugin-ckeditor5-rules.git
  • cd eslint-plugin-ckeditor5-rules
  • git checkout i/8581
  • yarn link (it should call yarn install)
  • cd ../
  • The same for the second repository.
  • git clone git@github.com:ckeditor/eslint-config-ckeditor5.git
  • cd eslint-config-ckeditor5
  • git checkout i/8581
  • yarn link (it should call yarn install)
  • cd ../
  • The main repository:
  • cd ckeditor5
  • yarn link eslint-plugin-ckeditor5-rules
  • yarn link eslint-config-ckeditor5

Then, on the dll-integration branch, yarn run lint should print some errors. The i/8581 branch should fix those errors.

@pomek pomek marked this pull request as ready for review January 8, 2021 13:11
@@ -16,10 +16,18 @@ export { default as Conversion } from './conversion/conversion';

export { default as HtmlDataProcessor } from './dataprocessor/htmldataprocessor';

export { default as InsertOperation } from './model/operation/insertoperation';
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to add these?

Copy link
Member Author

Choose a reason for hiding this comment

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

CF.

Copy link
Member

Choose a reason for hiding this comment

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

If we're adding InsertOperation we should also add all other operations here.

@Reinmar
Copy link
Member

Reinmar commented Jan 11, 2021

What about these?

image

@Reinmar
Copy link
Member

Reinmar commented Jan 12, 2021

image

So, my stance on this is that consistency is important. It will be confusing which packages use direct imports and which go via ckeditor5. We'd need additional ESLint rules to enforce that packages within the DLL use direct imports.

Therefore, I'd rather state it a bit differently: Every package that we have may only have three types of imports:

  • directly from within the same package (local ones)
  • from ckeditor5
  • from 3rd party things

Therefore, it'd be great if you could research how much work would require to enforce this rule. I think it may require adding a few more things to packages' index files but other than that it seems doable.

@Reinmar
Copy link
Member

Reinmar commented Jan 12, 2021

Oh, and one more thing – you can also consider my yesterday's finding that plugins that are part of the Essential plugin should be in the DLL.

@Reinmar Reinmar merged commit 4681215 into dll-integration Jan 12, 2021
@Reinmar Reinmar deleted the i/8581 branch January 12, 2021 13:37
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.

2 participants