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

Turn cloud-services-core into a soft requirement #8811

Closed
Reinmar opened this issue Jan 12, 2021 · 6 comments · Fixed by #9011
Closed

Turn cloud-services-core into a soft requirement #8811

Reinmar opened this issue Jan 12, 2021 · 6 comments · Fixed by #9011
Assignees
Labels
domain:extending-builds release:blocker This issue blocks the upcoming release (is critical). type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 12, 2021

Provide a description of the task

Right now it's part of the DLL. However, it's a bit problematic because it's not a commonly used package.

Some of my thoughts:

  • We need to turn this into a soft requirement.
  • We considered having two DLLs (the base one and a richer, collab-ready one with more packages included), but the fact that packages from DLL must be imported via ckeditor5 is a deal breaker because that'd mean that in the source files we would need to change imports from cloud-services-core to ckeditor5-based. This would in turn means that such packages need to be added to ckeditor5. While this is more or less fine for cloud-services-core which is part of the OSS part, this strategy with having two DLLs would work best if we could use this for commercial packages.
  • Also, if we'd want to use this collab-ready DLL for solving the problem with comments editor using e.g. basic styles, we'd need to also add basic-styles to the DLL (and hence ckeditor5. Same for the editor class. That would make it even more messy.
  • Anyway, this would be confusing too. It gets more complex the longer we think about this.
  • It seems that soft requirements, and perhaps some tricks to make them nice to use, is our only options. Soft requirement means a "dynamic requirement" and it's a bit like in the old RPM days when you knew you need to load some lib but the system would not install it automatically for you.

In order to turn it into a soft requirement of other packages:

  • We need to check which things from this packages are imported in other packages.
  • These things need to be somehow moved to the plugin(s) to make them available to other plugins.
  • Code that was directly importing these things previously needs to be updated to use these things via the plugins. These plugins need to be added as soft requirements of code that consumes them.
@Reinmar Reinmar added type:task This issue reports a chore (non-production change) and other types of "todos". domain:extending-builds squad:dx labels Jan 12, 2021
@Reinmar Reinmar added this to the iteration 39 milestone Jan 12, 2021
@Reinmar Reinmar self-assigned this Jan 12, 2021
@Reinmar
Copy link
Member Author

Reinmar commented Jan 12, 2021

Fortunately, there are just a few imports:

image

image

However, a package that is much more complex to handle:

image

But this package is only used in CF so we can tackle it later.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 12, 2021

Actually, from the above mentioned 5 imports of cloud-services-core we only need to fix in this issue these which are in the ckeditor5 repo. Turning ckeditor5-internal and collaboration-features into DLL-compatible plugins is a future step.

@Reinmar
Copy link
Member Author

Reinmar commented Jan 12, 2021

Oops. I confused cloud-services-core and cloud-services. We'll need to review once again how both packages are used.

@pomek
Copy link
Member

pomek commented Feb 11, 2021

While working on the issue, I've noticed that we have an editor that adds to its built-in plugin CloudServices plugins (which now requires CloudServicesCore as a soft requirement).

It meant that I had to add the new plugin (CloudServicesCore exposed by the @ckeditor/ckeditor-cloud-services-core package) to the editor as well.

After that, some tests of the editor stopped passing. I wasn't able to mock the services.

I was thinking about how to resolve the issue and together with @ma2ciek, we figured out an idea for introducing an option for replacing built-in plugins.

Thanks to that, we can replace a built-in plugin with a custom mock. There are some limitations that could be improved in the future but do not interfere with the current ticket:

  1. A plugin for replacing must be a class and must have a name (#pluginName must return a string).
  2. Both plugins ("mock" and the original) cannot have dependencies.
  3. Plugin that should be replaced must be loaded to the editor. You cannot replace a plugin that will not be loaded.

@ma2ciek
Copy link
Contributor

ma2ciek commented Feb 11, 2021

☝️ It would be quite cool if we would be able to replace plugins with dependencies as well, as it should allow making some major customizations deep in the architecture.

@pomek
Copy link
Member

pomek commented Feb 17, 2021

While working on the issue, we figured out that we don't need the package at all. It can be merged into the @ckeditor/ckeditor5-cloud-services package.

Notes from our internal meeting:

  • ckeditor-cloud-services-core
    • We can remove this package from GH.
    • We need to release a new empty major version of this package and use npm deprecate to make sure people are warned when they try to upgrade to this package.
    • The use case is that if a project is using 25.0.0 and use ncu to upgrade all packages, they should be warned when they try to upgrade to ckeditor-cloud-services-core in version 26.0.0. If we wouldn't release 26.0.0, they would have all packages updated (by ncu) except cloud-services-core and wouldn't learn that it got deprecated.
  • CloudServicesCore should be moved to ckeditor5-cloud-services and become a dependency (hard, not soft) of the CloudServices plugin.
  • The Token, UploadGateway should be moved too and should be available if someone needs to import them directly.
  • Let's keep working on the current branch so we can easily diff with master to see if we didn't leave some unnecessary changes (e.g. CloudServicesCore in some places).

@mlewand mlewand added the release:blocker This issue blocks the upcoming release (is critical). label Feb 23, 2021
Reinmar added a commit that referenced this issue Feb 23, 2021
Other (cloud-services-core): All classes available in the `@ckeditor/ckeditor-cloud-services-core` package have been moved to the `@ckeditor/ckeditor5-cloud-services` package. They should now be instantiated via factory methods on the `CloudServicesCore` plugin. Closes #8811.

Feature (cloud-services): Created the `CloudServicesCore` plugin that exposes the base API for communication with CKEditor Cloud Services.

MAJOR BREAKING CHANGE (cloud-services-core): The package has been merged into `@ckeditor/ckeditor5-cloud-services`. All classes that were available in the `@ckeditor/ckeditor-cloud-services-core` package have been moved to the `@ckeditor/ckeditor5-cloud-services` package. They should now be instantiated via factory methods on the `CloudServicesCore` plugin that's located in `@ckeditor/ckeditor5-cloud-services`. See #8811.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:extending-builds release:blocker This issue blocks the upcoming release (is critical). type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants