cli: Add check:theia-extensions command
#79
Conversation
| } else { | ||
| if (dependency.isTheiaExtension) { | ||
| matchingPackageJsons.push(dependency); | ||
| const childNodeModules: string = path.join(nodeModulesDir, packageJsonPath, `../${NODE_MODULES}`); |
There was a problem hiding this comment.
Why do we need to find the transitive dependencies "by hand" here?
There was a problem hiding this comment.
Thanks for the fast feedback!
So i choose to do it "by hand" here as i didn't want to change the functionality of the existing check, also it makes it easier to stop searching, when we hit a non theia extension. Would you expect something else here?
BTW, i found a small issue with the code here, while retesting. It should be path.join(nodeModulesDir, packageJsonPath, '..'); I will change this together with your other feedback.
There was a problem hiding this comment.
But in the old version, findDependencies was never recursively called, right? So I assumed that the full dependency tree would be computed through some other code path. Why the recursive call only for packages which contribute Theia extensions?
There was a problem hiding this comment.
So as far as i can tell, the old version simply looked at the first level of node_modules in root and all workspaces specified. To quickly test, i ran the check for all dependencies (not only theia extensions) and it actually does not show the versions mismatch found in node_modules/@eclipse-emfcloud/modelserver-theia/node_modules/@theia/core/package.json. So maybe this was simply overlooked?
I can adjust the code to recursively call findDependencies for each of the three use cases.
There was a problem hiding this comment.
Either way, methinks it makes sense to either always recurse or never.
There was a problem hiding this comment.
@tsmaeder any objections on opening the upstream PR?
There was a problem hiding this comment.
I'm not sure whether doing the check recursively is right: the code before did not do it. However, I don't really understand the reasoning. behind it. @planger, since you wrote the original code, can you chime in?
There was a problem hiding this comment.
I haven't looked into the requirements this PR aims to address yet. For now I can just say that -- as far as I remember -- we didn't need a recursive check before, because yarn install already does the recursive resolution for us and puts the result into the node_modules folders. Thus checking the node_modules folders is enough as it is the flattened list of all direct and indirect dependencies.
There was a problem hiding this comment.
What would happen, if we have the following dependency tree:
browser app:
- @theia/core: 1.38.0
- 3rd-party-A: 1.0.0
- 3rd-party-B: 1.0.0
- theia-extension
theia-extension:
- @theia/core: 1.37.0
- 3rd-party-A: 2.0.0
- 3rd-party-B: 2.0.0
3rd-party-A: 2.0.0:
- @theia/core: 1.36.0
- 3rd-party-B: 3.0.0
3rd-party-B: 3.0.0:
- @theia/core: 1.35.0
Wouldn't we then get:
node_modules
@theia/core: 1.38.0
3rd-party-A: 1.0.0
3rd-party-B: 1.0.0
test-extension:
node_modules:
@theia/core: 1.37.0
3rd-party-A: 2.0.0
3rd-party-B: 2.0.0:
node_modules:
@theia/core: 1.36.0
3rd-party-B: 3.0.0:
node_modules:
@theia/core: 1.35.0
It would then no longer be enough to check the first level of node_modules as @theia/core: 1.35.0 would be nested.
It could totally be the case, that i am not understanding something or missing a point here, i am just curios 🤔
There was a problem hiding this comment.
Yes you are right!
The current version on master only checks the root level and first level (where extensions and browser-app/electron-app typically are). If the dependencies are hoisted, however, checking the root level and the first level should imho always be already sufficient to at least identify that we've pulled multiple versions of the same dependency, because hoisting always starts from root to leaf, so at least one version would already be on root and one would be on level 1.
So turning this into a recursive check will imho not identify additional error cases, but enables giving more complete error messages; that is, it enables listing all pulled versions of a dependency, not just the first two versions (that happened to be in root and level 1). For non-hoisted cases, the situation may be completely different though -- I haven't tested with such scenarios yet.
In summary, if we want to be able to provide a complete error message (without having to run yarn why after the error was printed), the recursive check makes sense I guess.
Also keeping both checks theia check:theia-version (a) and your new one (b) makes sense, because (b) checks for uniqueness of packages, whereas (a) checks whether the exact same Theia version is used across all dependencies (also if they are unique but different).
|
@tsmaeder I updated the PR and also adjusted the |
This allows to check for duplicate versions of theia extensions only. Added the option `only-theia-extensions` to `theia check:dependencies`. Theia extensions are identified via their `theiaExtensions` field in the `package.json`. The check is running recursively as long as it finds Theia extensions. The advantage of this approach is that a shorter, but more important, list is returned. Contributed on behalf of STMicroelectronics
sgraband
force-pushed
the
12572-checkTheiaExtensions
branch
from
2c2b319
to
a113bd5
Compare
|
Closed in favor of eclipse-theia#12596 |
What it does
The goal of this PR is to improve the existing
theia check:dependenciesscript introduced in this PR. The existing script returns a list of all mismatching version numbers for all dependencies. While introducing two versions of the same dependency can always possibly introduce issues, this will be much more common for Theia extensions, than for other dependencies likeloadashorrimraf. Thats why:This allows to check for duplicate versions of theia extensions only.
Added the option
only-theia-extensionstotheia check:dependencies.Theia extensions are identified via their
theiaExtensionsfield in thepackage.json.The check is running recursively as long as it finds Theia extensions.
The recursion was added to also display cases where more than 2 versions of a dependency might extist.
The advantage of this approach is that a shorter, but more important, list is returned.
Fixes # 12572.
Contributed on behalf of STMicroelectronics
How to test
yarndev-packages/cliand runyarn linkyarn link "@theia/cli"in its rootyarnto see the output (this will runtheia check:theia-extensionson postinstall)The provided example has a few issues built-in:
test-extensionrequires a@theia/coreversion of1.37.2while the application depends on1.38.0test-extension>test-extension-3>@eclipse-emfcloud/modelserver-theia:0.8.0-theia-cr03test-extension2>test-extension-4>@eclipse-emfcloud/modelserver-theia:0.8.0-theia-cr01@eclipse-emfcloud/modelserver-theia:0.8.0-theia-cr01depends on version1.37.2of@theia/(core|filesystem|process|variable-resolver|workpace)So the result of
theia check:theia-extensionsshould find the following issues:@theia/(filesystem|process|variable-resolver|workpace)from the application and@eclipse-emfcloud/modelserver-theia@theia/corefrom the application,@eclipse-emfcloud/modelserver-theiaandtest-extension@eclipse-emfcloud/modelserver-theiafromtest-extension-3andtest-extension-4Note that one of the versions will always be hoisted to the root
node_modulesversionReview checklist
Reminder for reviewers