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

cli: Enhance tooling for checking dependencies #11483

Merged

Conversation

planger
Copy link
Contributor

@planger planger commented Jul 29, 2022

What it does

Enhances the theia CLI with dedicated checks regarding pulled dependencies as detailed below.

theia check:theia-version

Check that all dependencies have been resolved to the same Theia version.

It'll obtain all package workspaces of the monorepo from package.json/workspaces and run through their node_modules as well as the root node_modules in order to collect all dependencies to @theia/** packages and test whether they all have the same version. It'll exclude @theia/monaco-editor-core though, as it follows the version of Monaco, which differs from the common Theia version.

Parameters:

  • --suppress or -s: Suppress exiting with failure code
theia check:hoisted

Check that all dependencies are hoisted.

This command behaves as before, but uses the same enhanced implementation as the command above and below. Also the output is a bit improved with respect to readability in comparison to the current implementation.

theia check:dependencies

Check uniqueness of dependency versions or whether they are hoisted.

This command is the basis for the two commands above, but exposes the full configurability to adjust the dependency checks to specific needs.

Parameters:

  • --workspaces or -w: Glob patterns of workspaces to analyze, relative to cwd. [array] [default: All glob patterns listed in the package.json's workspaces]
  • --include or -i: Glob pattern of dependencies' package names to be included, e.g. -i "@theia/**". [array] [default: ["**"]]
  • --exclude or -e: Glob pattern of dependencies' package names to be excluded. [array] [default: None]
  • --skip-hoisted or -h: Skip checking whether dependencies are hoisted. [boolean] [default: false]
  • --skip-uniqueness or -u: Skip checking whether all dependencies are resolved to a unique version. [boolean] [default: false]
  • --skip-single-theia-version or -t: Skip checking whether all @theia/** dependencies are resolved to a single version. [boolean] [default: false]
  • --suppress or -s: Suppress exiting with failure code. [boolean] [default: false]

See #11482 for the problem this PR aims to address.

Contributed on behalf of STMicroelectronics.

How to test

There are three commands (see above) to test.
For theia check:hoisted it should be tested whether it prints the same amount of information as before. However, the content may look slightly different now:

image

To test yarn check:theia-version you could proceed as follows:

  • Check out this PR and build (yarn)

  • Go to dev-packages/cli and run yarn link

  • Create a sample Theia application, e.g. with the theia generator, and run yarn link "@theia/cli" in its root

  • Running yarn && yarn theia check:theia-version should give no errors:
    image

  • To test different Theia versions of non-overlapping dependencies, you could remove @theia/terminal from the dependencies of browser-app and electron-app, set all their dependencies to 1.28.0, but add @theia/terminal as a dependency with a 1.26.0 in a Theia extension of your project. Note how no @theia/* dependencies are hoisted. However, in the root node_modules you'll find a mix of versions in the @theia/* dependencies (terminal vs the others). Now run yarn && yarn theia check:theia-version again and it should print:
    image

  • You can now re-add @theia/terminal with version 1.28.0 to browser-app, which will also lead to non-hoisted dependencies and re-run yarn && yarn theia check:theia-version -s (suppress). You'll get now one error (the same error as above), but also a warning that there is a dependency that isn't hoisted. Also note how the exit code differs because of the -s:
    image

To test yarn check:dependencies you could proceed as above, but now play around with the parameters. Implementation-wise it is the same as the two commands above. Try e.g.
yarn theia check:dependencies --workspaces "packages/**" --exclude "rimraf/**" --skip-single-theia-version
which will find non-hoisted dependencies, only the packages of the folder packages, but will not show an error for rimraf.

Don't forget to yarn unlink in dev-packages/cli after testing.

Review checklist

Reminder for reviewers

@planger
Copy link
Contributor Author

planger commented Aug 9, 2022

Just wanted to ping if someone would be available to give feedback to this proposal? ☺️ Thanks a lot!

@msujew msujew self-requested a review August 9, 2022 12:42
@planger
Copy link
Contributor Author

planger commented Aug 22, 2022

@msujew Did you have a chance to look at this PR? I'm looking forward to your feedback. Thanks a lot!

@paul-marechal paul-marechal self-requested a review August 23, 2022 00:55
@planger planger force-pushed the planger/issues/11482 branch from a44661b to 1be07cc Compare August 24, 2022 13:23
@planger
Copy link
Contributor Author

planger commented Aug 24, 2022

@vince-fugnitto @paul-marechal Thanks a lot for your feedback! I pushed an update which includes your suggestions.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm that the changes work better 👍
I'll give @paul-marechal a chance to perform a final review as well.

dev-packages/cli/package.json Outdated Show resolved Hide resolved
@planger planger force-pushed the planger/issues/11482 branch from 1be07cc to 94ac974 Compare August 24, 2022 14:54
@paul-marechal
Copy link
Member

paul-marechal commented Aug 24, 2022

Using the following package.json

{
  "name": "theia-app-example",
  "version": "1.0.0",
  "main": "index.js",
  "license": "MIT",
  "dependencies": {
    "@theia/core": "^1.28.0",
    "@theia/plugin-ext": "^1.28.0",
    "@theia/plugin-ext-vscode": "^1.28.0"
  },
  "devDependencies": {
    "@theia/cli": "^1.28.0"
  }
}

Running theia check:dependencies seems to find errors that I believe shouldn't be picked up:

image

Note how these aren't nested dependencies... As a rule of thumb I think we should only consider packages like node_modules/[@package-namespace/]package-name/package.json.

@paul-marechal
Copy link
Member

Otherwise I tried polluting my example app by adding @theia/[email protected] to force a duplication and it was correctly picked up. I think it should be good to merge once my previous issue is fixed.

eclipse-theia#11482

Contributed on behalf of STMicroelectronics.

Change-Id: If442c9a135dc152082ec810713cf8de24ff2b451
Signed-off-by: Philip Langer <[email protected]>
@planger planger force-pushed the planger/issues/11482 branch from 94ac974 to 1eb5747 Compare August 24, 2022 16:40
@planger
Copy link
Contributor Author

planger commented Aug 24, 2022

Thanks again for your feedback!
@paul-marechal You're right, we should filter package.json's deeply nested in dependencies. I've now added these two ignore globs:

`[^@]*/*/**/${PACKAGE_JSON}`, // package.json that isn't at the package root (and not in an @org)
`@*/*/*/**/${PACKAGE_JSON}`, // package.json that isn't at the package root (and in an @org)

I confirmed that the dependencies you mentioned aren't showing up, while other expected ones still show up.
Can you confirm this works for your example too?

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successfully picks up duplicated Theia package version, this will be helpful!

@vince-fugnitto vince-fugnitto added the theia-cli issues related to the theia-cli label Aug 24, 2022
@martin-fleck-at martin-fleck-at merged commit 6bd8e74 into eclipse-theia:master Aug 25, 2022
@vince-fugnitto vince-fugnitto added this to the 1.29.0 milestone Aug 25, 2022
@planger planger deleted the planger/issues/11482 branch September 26, 2022 07:58
planger added a commit to eclipsesource/generator-theia-extension that referenced this pull request Sep 26, 2022
This will run the Theia version check after yarn install to check
whether multiple versions of @theia/* packages are pulled by direct or
indirect dependencies.

See also eclipse-theia/theia#11483
planger added a commit to eclipsesource/generator-theia-extension that referenced this pull request Sep 26, 2022
This will run the Theia version check after yarn install to check
whether multiple versions of @theia/* packages are pulled by direct or
indirect dependencies.

See also eclipse-theia/theia#11483

Contributed on behalf of STMicroelectronics.
vince-fugnitto pushed a commit to eclipse-theia/generator-theia-extension that referenced this pull request Oct 25, 2022
This will run the Theia version check after yarn install to check
whether multiple versions of @theia/* packages are pulled by direct or
indirect dependencies.

See also eclipse-theia/theia#11483

Contributed on behalf of STMicroelectronics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theia-cli issues related to the theia-cli
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants