How to enable two adapters in one build

[Reinmar]

#567 assumes that we'll have two adapters enabled in all builds – the CKFinder's and CS's ones.

Currently:

• If configured, they will override one another.
• If not configured, the CKF's one will log a warning. The CS's one should do the same (https://github.com/ckeditor/ckeditor5-easy-image/issues/5). Which means that if you'll have one adapter configured, you'll always see the other one complaining. If you won't have any of them configured, both will complain.

So, we have a problem here.

• One option is to not log any warning if the configuration is not specified. But this means that we give up the opportunity to help the developer configure a freshly installed plugin.
• The other option might be to log a warning in afterInit() if none of the adapters was configured (which can be discovered by the lack of createAdapter() callback). But this means that we'll be logging a warning if someone uses a CKEditor 5 Build but doesn't use the built-in upload functionality (which is often).

Both options are quite bad.

So, maybe we should consider a third option – that the adapters will be available in builds, but will not be enabled by default. You'd enable one by using e.g. config.extraPlugins = [ 'EasyImage' ]. However, this is quite a lot of work for us because we don't yet support building in "extra available plugins", nor config.extraPlugins.

If we had more time, I'd go with the 3rd option. But taken the time constraints the first option seems to be most feasible. However, it'd be good to have a long-term plan for that since going with 3rd option means doing breaking changes so we should schedule that for before 1.0.0.

cc @pjasiun @wwalc

PS. I skipped the problem with one adapter overriding the other one if both are configured. This is another problem and perhaps should be handled by a more capable factory – one which throws when overridden. I wonder, though, how important this issue may be. How many developers will, for some reason, configure both adapters. And I'm guessing that some will because C&P-based development is a popular approach.

[pjasiun]

I skipped the problem with one adapter overriding the other one if both are configured.

First of all, if one wants to have both adapters enabled he should have a special "adapter" provided which will handle it. I have no idea how it supposes to work: some file types handled by one adapter, some by another? That's an edge case.

So, maybe we should consider a third option – that the adapters will be available in builds, but will not be enabled by default. You'd enable one by using e.g. config.extraPlugins = [ 'EasyImage' ].

I wanted to say that this is a good idea, but then I realized that there no real difference between this and removing console.warn from the code: in both cases, the user will not have a warning at the beginning and need to find a tutorial to know how to start. From the technical point of view, I think that if we want to enable these plugins by default there should be no warning there.

However, the editor may seem to be incomplete without any adapter, because there is no way to insert an image if no adapter is defined.

Also, if each adapter will have its own warning it will be very misleading the user. Most probably he will not get that need to define only one and try to follow all warnings, wondering why he needs to configures that many URLs.

This is why we should check in the fileRepository plugin, in the afterInit listener, if there is any adapter defined and if no, console.warn a message how to configure adapter or disable this warning (by remove adapters plugins).

[Reinmar]

First of all, if one wants to have both adapters enabled he should have a special "adapter" provided which will handle it. I have no idea how it supposes to work: some file types handled by one adapter, some by another? That's an edge case.

You misunderstood that case. I was talking about developers who blindly or mistakenly configured both at the same time. This often happens if you do copy-paste from SO. We might want to warn those developers that they did something wrong.

[Reinmar]

I wanted to say that this is a good idea, but then I realized that there no real difference between this and removing console.warn from the code

Good point.

This is why we should check in the fileRepository plugin, in the afterInit listener, if there is any adapter defined and if no, console.warn a message how to configure adapter or disable this warning (by remove adapters plugins).

I agree that this is the right place. A common warning logged by the FileRepository with an explanation that you either need to configure one of the adapters or disable the plugins by (to avoid warnings if they bother you):

config.removePlugins = [ 'ImageUpload', 'EasyImage' ];

Thankfully we have error codes docs already (and log.warn() links to that page) so this will be fine.

[Reinmar]

Ok, this is how the error shows up:

[Reinmar]

So it seems to be done in https://github.com/ckeditor/ckeditor5-upload/issues/58.

But now I need to check whether all this works when used in builds.

[Reinmar]

Warning in the browser:

[Reinmar]

OK, muting this error doesn't work for two reasons:

• We also need to remove ImageUpload plugin from the build. Unfortunately, adding Easy Image and upload with CKF means that I need to add 3 plugins to each build – EI, ImageUpload and CKF's adapter.
• After I added ImageUpload to removePlugins the editor still loads FileRepository. I guess that removePlugins doesn't work correctly and doesn't prevent loading an unneeded dependency. I'll debug this tomorrow. It should be easy to fix.

[Reinmar]

I guess that removePlugins doesn't work correctly and doesn't prevent loading an unneeded dependency. I'll debug this tomorrow. It should be easy to fix.

Hm... It's not that. At least, it does not seem so. Weird.

[Reinmar]

OK, that's because ImageUpload wasn't a named plugin. Fixed in ckeditor/ckeditor5-upload@de6d575.

[fredck]

Got confused with the talks here, but will leave my opinion here.

There should be no warning if one uses our builds as is, with no configurations out of the box.

[Reinmar]

You're right. There should be no warning.

This means that we're back to square one.

So, which option is better?

• Should both features be enabled by default in all builds? In this case, we must not log any warning when they are not configured, which means that we won't be able to help a developer with what's the next step adfter initializing the editor.
• Should both features be included in a build but disabled by default? A developer would be able to enable them using config.extraPlugins (at this stage, a warning will start to show up). This case is very similar to the previous one, but we'll at least be able to help those who figured out that a plugin needs to be enabled first and all developers who created custom builds/integrations.

Above, we're talking about upload adapters. They are invisible to the user so, when not configured, they don't affect the editor in any way.

However, there's also ImageButton plugin. First of all, do we want to add it to the builds? Second, I presume that it should warn when upload adapter is not configured?

[Reinmar]

I'm for the first solution because it's simpler. The second one may be confusing because enabling a feature will require doing two things – enabling it and then configuring.

However, generally speaking, proposing two upload adapters enabled in one build sounds bad to me ;/. But I guess we can't solve this in a batter way anyway.

[fredck]

Should both features be enabled by default in all builds?

I would say "yes". Developers should be a few configurations away from having them working and I feel that the extraPlugins configuration is not what we're talking about. It should feel like magic.

In this case, we must not log any warning when they are not configured, which means that we won't be able to help a developer with what's the next step after initializing the editor.

Yes, no warning if any configuration is made. Developers will rely on the documentation for that, not on a console warning. Actually, no matter the option, the developer will always look at the documentation first (in the second option, to know that the plugin must be enabled), so the warning is really pointless, just like as having to enable the plugin manually (which just becomes an additional step to have things done).

However, there's also ImageButton plugin. First of all, do we want to add it to the builds?

The original idea is that we don't have it. It should definitely not show up if EI is enabled, that's for sure.

Second, I presume that it should warn when upload adapter is not configured?

If the button is configured to be in the toolbar, then probably showing a warning is a good idea... or maybe an error when the button is clicked.

[Reinmar]

The original idea is that we don't have it. It should definitely not show up if EI is enabled, that's for sure.

Why do you think that EI + ImageButton makes no sense? 90% of the time I'm using the "upload" button in Slack to pick an image.

[Reinmar]

If the button is configured to be in the toolbar, then probably showing a warning is a good idea... or maybe an error when the button is clicked.

I'll check if that wouldn't happen now. I think that a warning will be logged.

[Reinmar]

Not sure if this is ok, but if we won't log a warning on startup then an error may still be logged when one pastes or drops an image. Is this acceptable? Sounds poorly to me ;/

This happens because the feature is unfortunately enabled. It just can't upload the image.

[Reinmar]

Not sure if this is ok, but if we won't log a warning on startup then an error may still be logged when one pastes or drops an image. Is this acceptable? Sounds poorly to me ;/

The only alternative I can see is logging a warning specific for ImageUpload instead of the error that we get now (https://github.com/ckeditor/ckeditor5-upload/issues/59). Which is a small improvement anyway.

So, I'm going with this.

[fredck]

Why do you think that EI + ImageButton makes no sense? 90% of the time I'm using the "upload" button in Slack to pick an image.

Sorry, my mistake. I was way too focused on thinking about the file manager on that button that I forgot about the file system integration.

Ofc the button makes sense with EI as well, but not in a default setup.

[fredck]

Not sure if this is ok, but if we won't log a warning on startup then an error may still be logged when one pastes or drops an image. Is this acceptable? Sounds poorly to me ;/

This happens because the feature is unfortunately enabled. It just can't upload the image.

I don't think any error or warning should be ever shown. And in fact, although the plugin is loaded, the feature should not be enabled if there is no configuration set for it.

[Reinmar]

OK, lacking time we agreed to not polish the cases in which warning is logged any further now. Especially, that there are little details which matter here like the fact that ImageUploadButton will be enabled in the builds despite the fact that we didn't list it (cause for some reason it's ImageUpload's depdency).

I reported https://github.com/ckeditor/ckeditor5-upload/issues/61 for the future.

[Reinmar]

The status of things now is that an error is logged when image is dropped or pasted and when using the "Insert image" button.

[lavantrinh]

i have same error, please help me