search

ckeditor / ckeditor4-react

Official CKEditor 4 React component.
Other
97 stars 52 forks source link

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

Open f1ames opened 4 years ago

f1ames commented 4 years ago

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)?

jacekbogdanski commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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.

MMMalik commented 3 years ago

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?