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

Should CKEditor 4 be really a dependency of React integration #103

Open
f1ames opened this issue Jun 18, 2020 · 4 comments
Open

Should CKEditor 4 be really a dependency of React integration #103

f1ames opened this issue Jun 18, 2020 · 4 comments
Assignees
Labels
status:confirmed An issue confirmed by the development team. type:task Any other issue (refactoring, typo fix, etc).

Comments

@f1ames
Copy link
Contributor

f1ames commented Jun 18, 2020

Are you reporting a feature request or a bug?

Task

Provide detailed reproduction steps (if any)

For CKEditor 4 React integration (ckeditor4-react) ckeditor4 is both peer and dev dependency, while in other integrations - Angular and Vue, ckeditor4 is not any dependency at all.

Since all the integrations works in the same manner regarding CKEditor 4 usage (by default fetching from CDN which can be changed by passing custom URL and also can use CKEDITOR namespace if globally present) I wonder if it's a correct approach here (or maybe Angular and Vue do something wrong)?

@f1ames f1ames added status:pending More information is needed to proceed with the issue. type:task Any other issue (refactoring, typo fix, etc). workload:low labels Jun 18, 2020
@jacekbogdanski
Copy link
Member

If we are not using this dependency in the codebase (e.g. for unit tests) I'm not sure what's the reason to keep it in the package dependencies. I'm for removing it if it's not required.

@Comandeer
Copy link
Member

Actually I'm for switching unit test to local copy of CKE4, not the one from CDN. Unit tests should not depend on anything that is out of our control (and CDN is such a thing).

Additionally I think that making CKE4 a peer dependency clearly shows that the component needs CKE4 to work correctly.

@jacekbogdanski
Copy link
Member

@Comandeer that makes a lot of sense, indeed using CDN for unit testing makes more like integration tests, not units. Although, we still need at least one integration test checking out if CDN URL is correct. I suppose we will have to do some tests refactoring due to this issue.

@jacekbogdanski jacekbogdanski added status:confirmed An issue confirmed by the development team. and removed status:pending More information is needed to proceed with the issue. labels Jul 24, 2020
@MMMalik
Copy link
Contributor

MMMalik commented Aug 3, 2021

Regarding ckeditor4 as peer dependency. My take on this in React v2 was to treat ckeditor4 as optional peer dependency. It indicates that ckeditor4-react might use local copy of ckeditor4 but it will also work without explicitly installing it. peerDependenciesMeta is supported by e.g. npm@7, yarn and pnpm.

Regarding ckeditor4 as dev dependency. As long as we don't rely on local copy of ckeditor4 for testing then I believe it should be removed from devDependencies.

Actually I'm for switching unit test to local copy of CKE4, not the one from CDN. Unit tests should not depend on anything that is out of our control (and CDN is such a thing).

This might be a good approach but it would require us to run a separate server just to serve ckeditor4 and its dependencies. Anyway, it's probably a candidate for a different task.

To sum up, my suggestion is to remove ckeditor4 from dev dependencies and leave it as an optional peer dependency. Then we can close this issue. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:confirmed An issue confirmed by the development team. type:task Any other issue (refactoring, typo fix, etc).
Projects
None yet
Development

No branches or pull requests

4 participants