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

Invalid dependencies in ckeditor5-theme-lark - all deps should be dev deps #9998

Closed
mlewand opened this issue Jun 30, 2021 · 10 comments · Fixed by #10140
Closed

Invalid dependencies in ckeditor5-theme-lark - all deps should be dev deps #9998

mlewand opened this issue Jun 30, 2021 · 10 comments · Fixed by #10140
Assignees
Labels
intro Good first ticket. squad:devops Issue to be handled by the Devops team. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@mlewand
Copy link
Contributor

mlewand commented Jun 30, 2021

Provide a description of the task

In theme lark:

"@ckeditor/ckeditor5-basic-styles": "^28.0.0",
"@ckeditor/ckeditor5-ckfinder": "^28.0.0",
"@ckeditor/ckeditor5-code-block": "^28.0.0",
"@ckeditor/ckeditor5-core": "^28.0.0",
"@ckeditor/ckeditor5-editor-balloon": "^28.0.0",
"@ckeditor/ckeditor5-editor-classic": "^28.0.0",
"@ckeditor/ckeditor5-find-and-replace": "^0.0.1",
"@ckeditor/ckeditor5-font": "^28.0.0",
"@ckeditor/ckeditor5-heading": "^28.0.0",
"@ckeditor/ckeditor5-highlight": "^28.0.0",
"@ckeditor/ckeditor5-html-embed": "^28.0.0",
"@ckeditor/ckeditor5-indent": "^28.0.0",
"@ckeditor/ckeditor5-link": "^28.0.0",
"@ckeditor/ckeditor5-list": "^28.0.0",
"@ckeditor/ckeditor5-media-embed": "^28.0.0",
"@ckeditor/ckeditor5-page-break": "^28.0.0",
"@ckeditor/ckeditor5-paragraph": "^28.0.0",
"@ckeditor/ckeditor5-remove-format": "^28.0.0",
"@ckeditor/ckeditor5-restricted-editing": "^28.0.0",
"@ckeditor/ckeditor5-select-all": "^28.0.0",
"@ckeditor/ckeditor5-source-editing": "^0.0.1",
"@ckeditor/ckeditor5-special-characters": "^28.0.0",
"@ckeditor/ckeditor5-ui": "^28.0.0",
"@ckeditor/ckeditor5-undo": "^28.0.0",
"@ckeditor/ckeditor5-utils": "^28.0.0",
"@ckeditor/ckeditor5-table": "^28.0.0",

We have all the deps marked as regular dependencies, but we use them only in manual test - so that's purely dev dep. Not sure if that's true for all the packages, but certainly most of them, like ckeditor5-find-and-replace.

@mlewand mlewand added type:task This issue reports a chore (non-production change) and other types of "todos". intro Good first ticket. squad:devops Issue to be handled by the Devops team. labels Jun 30, 2021
@pomek pomek modified the milestones: backlog, iteration 45 Jul 12, 2021
@przemyslaw-zan przemyslaw-zan self-assigned this Jul 13, 2021
@przemyslaw-zan
Copy link
Member

Seems like these files are in fact in use in ckeditor5-theme-lark/theme/

@przemyslaw-zan przemyslaw-zan added the pending:feedback This issue is blocked by necessary feedback. label Jul 13, 2021
@mlewand
Copy link
Contributor Author

mlewand commented Jul 13, 2021

The question is why does our validator says that? Let's take a single example @ckeditor/ckeditor5-media-embed package.

I made a ctrl+shif+f search in theme package and the only match is in this line:

import media from '@ckeditor/ckeditor5-media-embed/theme/icons/media.svg';

That's a manual test so it's a dev dep. Is there any other media embed usage in there?

@przemyslaw-zan
Copy link
Member

.ck.ck-code-block-dropdown .ck-dropdown__panel {

Perhaps references in css files cause that?

@psmyrek
Copy link
Contributor

psmyrek commented Jul 13, 2021

Currently, the decision if given package should belong to dependencies or devDependencies depends on this function:

function isDevDependency( absolutePaths ) {
    return !absolutePaths.some( absolutePath => absolutePath.match( /src|theme/ ) );
}

A package is a devDependency if it is not used in the source and theme.

The bug here is because the absolutePath for ckeditor5-theme-lark package already contains the theme word (from the package name), so all packages imported in the ckeditor5-theme-lark will always be classified as dependencies.

@pomek
Copy link
Member

pomek commented Jul 13, 2021

Updating the RegExp to check only directories would resolve the problem. Keep in mind that the RegExp should work on both environments: Unix-like (/) and Windows (\).

psmyrek added a commit to ckeditor/ckeditor5-dev that referenced this issue Jul 13, 2021
Fix (tests): Prevented `isDevDependency()` function from reading "theme" word from the `ckeditor5-theme-lark` package name. See ckeditor/ckeditor5#9998.
@pomek
Copy link
Member

pomek commented Jul 13, 2021

Fix for the dep-checker was released: https://github.com/ckeditor/ckeditor5-dev/releases/tag/v25.2.6.

@pomek pomek closed this as completed Jul 13, 2021
@pomek pomek reopened this Jul 13, 2021
@pomek
Copy link
Member

pomek commented Jul 13, 2021

Miss click.

@przemyslaw-zan
Copy link
Member

Fix is in, but some dependencies are still marked as misplaced

@pomek
Copy link
Member

pomek commented Jul 13, 2021

On Unix-like OS:

image

@przemyslaw-zan
Copy link
Member

The following packages are used in the source and should be moved to `dependencies`
- @ckeditor/ckeditor5-basic-styles
- @ckeditor/ckeditor5-core
- @ckeditor/ckeditor5-utils

On Windows, these 3 dependencies still refuse to be moved into dev dependencies, but this will probably be a new ticket.

pomek added a commit that referenced this issue Jul 13, 2021
Internal (theme-lark): Moved all packages except for `@ckeditor/ckeditor5-ui` from dependencies to dev-dependencies . Closes #9998.
pomek added a commit that referenced this issue Jul 13, 2021
Internal (theme-lark): Moved all packages except for `@ckeditor/ckeditor5-ui` from dependencies to dev-dependencies . Closes #9998.
@mlewand mlewand removed the pending:feedback This issue is blocked by necessary feedback. label Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intro Good first ticket. squad:devops Issue to be handled by the Devops team. 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