Ckeditor5: Issues after introducing new dependency check

Created on 28 Mar 2019 · 18Comments · Source: ckeditor/ckeditor5

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

There are two issues here:

eslint-plugin-ckeditor5-rules – dunno.
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

bug

Source

Reinmar

Most helpful comment

It might be:

Bug in depcheck
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.

jodator on 10 Apr 2019

👍2

All 18 comments

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.

jodator on 28 Mar 2019

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 on 29 Mar 2019

Ad 2.

:+1: for creating a new solution. From scratch.

pomek on 29 Mar 2019

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 on 29 Mar 2019

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?

Reinmar on 29 Mar 2019

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.

oleq on 29 Mar 2019

👍1

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

Reinmar on 29 Mar 2019

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.

jodator on 29 Mar 2019

👍1

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

Reinmar on 5 Apr 2019

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 on 5 Apr 2019

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

cd /tmp
git clone [email protected]: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.

pomek on 10 Apr 2019

It might be:

Bug in depcheck
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.

jodator on 10 Apr 2019

👍2

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 on 10 Apr 2019

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():

pomek on 10 Apr 2019

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?

Reinmar on 15 Apr 2019

👍1

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

Checking dependencies...
All dependencies are defined correctly.

pomek on 17 Apr 2019

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

pomek on 17 Apr 2019

All PRs are closed now.

Reinmar on 23 Apr 2019

Was this page helpful?

0 / 5 - 0 ratings

Related issues

The "Bold" icon is not centered

Reinmar · 3Comments

How to enable SimpleUploadAdapter on this build?

metalelf0 · 3Comments

The requested toolbar item is unavailable.

MansoorJafari · 3Comments

Research CSS flex box layout in the UI

oleq · 3Comments

Should ImageButton be included in the builds?

Reinmar · 3Comments