Issues after introducing new dependency check

[Reinmar]

https://travis-ci.org/ckeditor/ckeditor5-editor-decoupled/builds/512456662#L2221

There are two issues here:

1. eslint-plugin-ckeditor5-rules – dunno.
2. Missing theme-lark – this is a bigger problem. This pkg is not directly imported by any of our packages (except builds and cke5) because we don't import anything from it. However, as I understand, it's actually used by our theme importer if no other skin is defined. Therefore, it throws on CI when we're trying to run tests without theme-lark installed at all. We should add it at some point in that CI test env. BTW, I'm starting considering whether we shouldn't use the actual ckeditor5 repo in each package's CIs. To have the same situation that we have when running tests from ckeditor5. Then, we would also be able to validate e.g. docs. It's not the first problem that we had due to the hacky way we run package tests on CI, so perhaps it's high time to really rethink and clean this.

cc @jodator @pomek

[jodator]

I don't have a clue why eslint-plugin-ckeditor5-rules is in that report. It is required by eslint-config-ckeditor5. I can't find any other references to it.

---

For imports it looks like missing dependency / wrong placement of _rwd.css mixin.

The textalternative.css imports from theme-lark: https://github.com/ckeditor/ckeditor5-image/blob/3cb2250835b736cc0cee6447a6541843eb95253e/theme/textalternativeform.css#L6 so it should require theme-lark.

But OTOH requiring optional theme-lark looks like a bug to me. This mixing should be moved to ui if features uses those mixins.

I've also found these imports (total 6 in 4 repos):

Targets
    Occurrences of 'import "@ckeditor/ckeditor5-theme-lark' in Directory /home/jodator/projects/cksource/ckeditor5/git/ckeditor5/packages
Found Occurrences  (6 usages found)
    Production  (4 usages found)
        Usage in string constants  (4 usages found)
            ckeditor5  (4 usages found)
                packages/ckeditor5-image/theme  (1 usage found)
                    textalternativeform.css  (1 usage found)
                        6 @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";
                packages/ckeditor5-link/theme  (2 usages found)
                    linkactions.css  (1 usage found)
                        6 @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";
                    linkform.css  (1 usage found)
                        6 @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";
                packages/ckeditor5-media-embed/theme  (1 usage found)
                    mediaform.css  (1 usage found)
                        6 @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";
    Test  (2 usages found)
        Usage in string constants  (2 usages found)
            ckeditor5  (2 usages found)
                packages/ckeditor5-engine/tests/manual  (2 usages found)
                    nestededitable.css  (2 usages found)
                        6 @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_focus.css";
                        7 @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_shadow.css";

In other words it looks like we're finding more issues with our hacky package testing desing :D

---

Anyway there are bunch of issues with testing single packages.

Also introducing changes to the CI should be tested on all packages not only randomly picked as this leads to breaking up CI :( But I don't know if it is feasible at all because there's 20+ packages to check with new setup.

[pomek]

Ad 1.

My local check:

CI:

Could we agree that eslint-plugin-ckeditor5-rules should be added to ignored packages? (https://github.com/ckeditor/ckeditor5-dev/blob/6b754f6a38f19ebe9ed456d98d97064e47c1c34d/packages/ckeditor5-dev-tests/bin/check-dependencies.js#L32)

[pomek]

Ad 2.

👍 for creating a new solution. From scratch.

[Reinmar]

Could we agree that eslint-plugin-ckeditor5-rules should be added to ignored packages? (https://github.com/ckeditor/ckeditor5-dev/blob/6b754f6a38f19ebe9ed456d98d97064e47c1c34d/packages/ckeditor5-dev-tests/bin/check-dependencies.js#L32)

Right after we figure out why it's listed. Because the first thing is to understand what's going on.

[Reinmar]

But OTOH requiring optional theme-lark looks like a bug to me. This mixing should be moved to ui if features uses those mixins.

cc @oleq. Do you think that non-optional CSS mixins could be moved to UI?

[oleq]

These are the places that import stuff from theme-lark directly

/Users/oleq/ck/5/ckeditor5/packages/ckeditor5-engine/tests/manual/nestededitable.css
  6,1: @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_focus.css";
  7,1: @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_shadow.css";

/Users/oleq/ck/5/ckeditor5/packages/ckeditor5-image/theme/textalternativeform.css
  6,1: @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";

/Users/oleq/ck/5/ckeditor5/packages/ckeditor5-link/theme/linkactions.css
  6,1: @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";

/Users/oleq/ck/5/ckeditor5/packages/ckeditor5-link/theme/linkform.css
  6,1: @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";

/Users/oleq/ck/5/ckeditor5/packages/ckeditor5-media-embed/theme/mediaform.css
  6,1: @import "@ckeditor/ckeditor5-theme-lark/theme/mixins/_rwd.css";

First two are in a manual tests so I guess we can ignore that (?). The rest is about _rwd.css. I'm totally OK with moving it to ckeditor5-ui.

[Reinmar]

First two are in a manual tests so I guess we can ignore that (?).

Then, I'd add theme-lark as a dev dependency of the engine, to avoid potential issues. After all, it's engine's dependency.

If we'll move _rwd to the UI... then we'd need to add UI as a dep of these packages which use it. Fortunately, it is a dep of these packages already, so no problem here. But if that would not be true, then adding it would make the dep check complain again :D And we'd need to mark theme-lark in depcheckIgnore in package.json. 👈 just saying so you know what to do in the future :D

[jodator]

If we'll move _rwd to the UI... then we'd need to add UI as a dep of these packages which use it. Fortunately, it is a dep of these packages already, so no problem here. But if that would not be true, then adding it would make the dep check complain again :D And we'd need to mark theme-lark in depcheckIgnore in package.json. point_left just saying so you know what to do in the future :D

Still - I think if something from a package's files (CSS file here) imports something from other package then it must have this that package in it's pacakge.json [dev]dependencies.

[Reinmar]

I added eslint-plugin-ckeditor5-rules to depcheckIgnore but it isn't the only problem:

https://travis-ci.org/ckeditor/ckeditor5-engine/builds/516111400

For some reason, builds started to timeout after Browserstack's tests are executed.

Plus, another issue is that the depcheck should actually kill the build but it does not – tests are executed even when the depcheck returned some incorrect deps:

https://travis-ci.org/ckeditor/ckeditor5-engine/builds/516099011

[pomek]

Plus, another issue is that the depcheck should actually kill the build but it does not – tests are executed even when the depcheck returned some incorrect deps:

https://github.com/ckeditor/ckeditor5-dev/blob/c353e23e5c8bb5b2691d94aae3f3b70e8fc3fd7f/packages/ckeditor5-dev-tests/bin/test-travis.sh#L14-L19

Missing '&& \' after L16. Then L17 and L18 should be merged.

[pomek]

Right after we figure out why it's listed. Because the first thing is to understand what's going on.

cd /tmp
git clone git@github.com:ckeditor/ckeditor5-core.git
cd ckeditor5-core/
yarn add @ckeditor/ckeditor5-dev-tests
node ./node_modules/.bin/ckeditor5-dev-tests-check-dependencies

produces:

Checking dependencies...
Found some issue with dependencies.

┌────────────────────────┬───────────────────────────────┬─────────────────────────┬─────────────────────┬────────────────────────┐
│ Invalid itself imports │ Missing dependencies          │ Missing devDependencies │ Unused dependencies │ Unused devDependencies │
├────────────────────────┼───────────────────────────────┼─────────────────────────┼─────────────────────┼────────────────────────┤
│                        │ eslint-plugin-ckeditor5-rules │                         │                     │                        │
└────────────────────────┴───────────────────────────────┴─────────────────────────┴─────────────────────┴────────────────────────┘

It could mean we use some kind of side-effect in CKE5 environment and for some reason, depcheck does not detect the package.

[jodator]

It might be:

1. Bug in depcheck
2. Our fault.

The output from dep-check is:

 missing: 
   { 'eslint-plugin-ckeditor5-rules': 
      [ '/some/path/ckeditor5/git/ckeditor5/.eslintrc.js' ],
...

Meaning that depcheck detects that .eslintrc.js load the eslint-plugin-ckeditor5-rules.

The file itself is:

module.exports = {
	extends: 'ckeditor5'
};

which only extends the base config eslint-config-ckeditor5 which is defined in package.json.

The eslint-config-ckeditor5 uses eslint-plugin-ckeditor5-rules plugin and have this plugin defined in package.json (otherwise it would not install & the eslint would fail).

Which boils down to previous two points: either bug in depcheck (should be ignored) or our fault for not including this plugin in package.json. The latter is in my opinion worse solution.

I'm for adding the eslint-plugin-ckeditor5-rules to ignored packages as the setup works as the less we write in package.json the better.

[pomek]

Plus, another issue is that the depcheck should actually kill the build but it does not – tests are executed even when the depcheck returned some incorrect deps:

https://github.com/ckeditor/ckeditor5-dev/blob/c353e23e5c8bb5b2691d94aae3f3b70e8fc3fd7f/packages/ckeditor5-dev-tests/bin/test-travis.sh#L14-L19

Missing '&& \' after L16. Then L17 and L18 should be merged.

It fixes the problem.

[pomek]

Plus, another issue is that the depcheck should actually kill the build but it does not – tests are executed even when the depcheck returned some incorrect deps:

After adding some process.exit():

[Reinmar]

We need the final decision and verification that _rwd.css should be moved to ckeditor5-ui and that there are no other dependencies on theme-lark in other packages. @pomek, could you check that and move that file too?

[pomek]

After merging PRs listed above, dep-checker should not display any error. Mgit returns 43x:

Checking dependencies...
All dependencies are defined correctly.

[pomek]

I am opening this ticket once again in order to not forget about those PRs.

[Reinmar]

All PRs are closed now.