Replace CDN with submodule

[jacekbogdanski]

It looks like we could benefit from using submodule instead of CDN - it will be easier to perform release tests and verify integration for different CKEditor version.

The issue is with Karma - it's possible to host local files although it seems like CKEditor requires a correct order of js files when using with ckeditor-dev.

Some configuration to start:

module.exports = function (config) {
    config.set({
        basePath: '../',
        frameworks: ['jasmine', '@angular-devkit/build-angular'],
        plugins: [
            require('karma-jasmine'),
            require('karma-chrome-launcher'),
            require('karma-jasmine-html-reporter'),
            require('karma-coverage-istanbul-reporter'),
            require('@angular-devkit/build-angular/plugins/karma')
        ],
        files: [
            { pattern: 'ckeditor-dev/ckeditor.js', included: true, served: false, watched: false },
            { pattern: 'ckeditor-dev/+(core|plugins|skins|lang)/**/*', included: true, served: false, watched: false },
            { pattern: 'ckeditor-dev/+(config|styles).js', included: true, served: false, watched: false },
            { pattern: 'ckeditor-dev/contents.css', included: true, served: false, watched: false },
        ],
        client: {
            clearContext: false // leave Jasmine Spec Runner output visible in browser
        },
        coverageIstanbulReporter: {
            dir: require('path').join(__dirname, '../coverage'),
            reports: ['html', 'lcovonly'],
            fixWebpackSourcePaths: true
        },
        reporters: ['progress', 'kjhtml'],
        port: 9876,
        colors: true,
        logLevel: config.LOG_INFO,
        autoWatch: true,
        browsers: ['Chrome'],
        singleRun: false
    });
};

[mlewand]

I'm a little bit unsure about that approach.

Fetching from CDN is more desired from user point of view. It caches better, saves the transfer and all the good stuff.

I see your reason for putting a submodule, but wouldn't it be just better to add ckeditor(-dev) as npm devDependency instead? And leave cdn to be used on the frontend, but Karma could load lib from npm.

[f1ames]

I think it could be done in a similar manner as we have in React integration. So we have ckeditor as devDependency and peerDependency and then also CDN. So if there is local ckeditor package it is used and if not integration uses CDN. WDYT @jacekbogdanski @engineering-this?

[jacekbogdanski]

Fetching from CDN is more desired from user point of view. It caches better, saves the transfer and all the good stuff.

The issue is not about the users (in that case CDN is fine) but integration testing before CKEditor4 release.

@f1ames if with that approach we will avoid configuring karma files, it would be great :+1:

[mlewand]

Yea, in this case dev dependency sounds like a good solution for that particular problem 🙂

[f1ames]

if with that approach we will avoid configuring karma files, it would be great 👍

@jacekbogdanski, I had to recheck this, from what I see, karma.conf.js in the React integration also uses files from CDN 🤔And it seems to not be using dev package but the CDN CKEditor version while running tests.

So here adding devDependency and adjusting Karma configuration as proposed in the initial comment should do the job (AFAIR the integration will use local CKEditor lib if available before querying CDN).

[f1ames]

@jacekbogdanski would you like to make a PR as you already started digging into this topic? 😃

[f1ames]

Now I started to wonder if running tests with ckeditor-dev is a good approach 🤔

React integration in most cases will be used with built version of CKEditor so I suppose tests should be also run with the built one. So one may use CDN or locally build (and providing url to ckeditor.js the same way the CDN url is provided).

it will be easier to perform release tests

It will be easier indeed as it is easier to change/switch local ckeditor-dev package. However, considering the above (tests should be run with the build version), there will be one additional step (building your local ckeditor-dev) and just providing the url to local build ckeditor.js instead of CDN url. Also the release test (if you mean integration release tests) are run with already released CKEditor version so it doesn't matter in this case.

verify integration for different CKEditor version

You can do the same with CDN links (unless you meant future versions, but then you can provide local builds).

So basically this change will be useful to quickly test some local changes in ckeditor-dev with the integration, but I'm not sure I we will need to do this very often.

WDYT @jacekbogdanski @mlewand ?

---

Btw. I have noticed the CDN link is not present in karma.conf.js, but it can be done the same way as in React integration.

And the alternative could be to provide some config/switch - by default tests will be run from CDN and by providing additional upon running tests, the local ckeditor-dev could be used.

[jacekbogdanski]

Actually, what's the release plan for integrations? We could explicitly pin down supported CKEditor4 versions by integration (docs etc.) and bump it after the editor release week or later when fully tested with CDN. In that case, there is no need to include a dev version.

If we want to release both CKE4 and integrations at the same time, we may lack CDN for the latest release during the testing phase.

About release version with ckeditor-dev - I was actually wondering some time ago why we are building presets one by one for the testing phase when ckeditor-dev could have dist or build folder with the latest CKE4 version (according to the latest / current release). Maybe it's the way to fix the issue with lacking production version for ckeditor-dev?

[f1ames]

Actually, what's the release plan for integrations?

Until now the integrations were released separately. It will be good to release the new package with bumped ckeditor dependencies (and CDN link) after each CKEditor release. But there also will be the cases when code of the integration is changed/updated and release will be not synchronized with CKEditor release. So both apply.

If we want to release both CKE4 and integrations at the same time, we may lack CDN for the latest release during the testing phase.

Hmmm, I wonder, have we tested integrations during release phase with newest version of CKEditor? If not we could do this, but we will need some manual tests (or just use samples). Still build version should be used here and integration uses CKEDITOR namespace if it's available so it's the matter of proper tests/samples with already loaded editor.

About release version with ckeditor-dev - I was actually wondering some time ago why we are building presets one by one for the testing phase when ckeditor-dev could have dist or build folder with the latest CKE4 version (according to the latest / current release). Maybe it's the way to fix the issue with lacking production version for ckeditor-dev?

Not sure I understand correctly? You would like to have CKEditor build included in ckeditor-dev? You can build it there by running build script. I think we shouldn't glue these both together, ckeditor-dev is a development repo (with possibility to build ofc) and presets are to provide standardized build. It could somehow solve this issue, but looks like a way around IMHO.

[engineering-this]

Hmmm, I wonder, have we tested integrations during release phase with newest version of CKEditor? If not we could do this, but we will need some manual tests (or just use samples). Still build version should be used here and integration uses CKEDITOR namespace if it's available so it's the matter of proper tests/samples with already loaded editor.

There is one tricky case to consider.

When publishing CKEditor docs, they serve CKEDITOR namespace with the latest release in examples section. Each of our frameworks will use CKEDITOR included by docs, so we might publish docs without even checking if framework integration works with the new version.

However I think it is very very unlikely that CKEditor update will somehow brake integration.

[jacekbogdanski]

Yes, I'm talking about built version included into ckeditor-dev during release, which could be used for CKE4 and integrations testing phase. But yeah, it may be redundant.

There is a case when we may like to test integrations before release, where some CKE4 changes break the integration and there is still a time to avoid it because the editor is not released yet.

[f1ames]

Ok, so from my perspective:

However I think it is very very unlikely that CKEditor update will somehow brake integration.

I agree with @engineering-this on this one. Now one of the easiest ways to check if integrations works with new CKEditor version is through docs (were we have integration samples). I was sure that the building docs is on of a pre-testing phase step, but couldn't find it in the dev docs ATM, I will add it to make sure it is always done so we will be sure nothing breaks. Also integration uses public API, as we rarely introduce any breaking changes the chances are quite low that CKEditor release will break the integration. I would be more concerned about updating React/Angular dependency which has higher probability of breaking integrations IMHO.

I think we should align testing flow for both React/Angular utilising CDN. And if there will be need in future for testing with ckeditor-dev (and we will get tired of manually adjusting Karma config or building CKEditor only to test integrations) we can introduce an option to switch between CDN or local dev CKEditor package while running tests. WDYT?

[f1ames]

Thanks to #15 Angular integration utilizes CI. We will see how it goes and improve the development flow if needed. Leaving this open for future reference.

[f1ames]

The #35 PR added possibility to pass custom CKEditor 4 url file to testing process which solves the case we wanted to cover with this issue.