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.

Sign up for GitHub

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

Jump to bottom

node: Represent workspace submodules as Projects #9247

Merged
merged 6 commits into from
Oct 9, 2024
Merged

node: Represent workspace submodules as Projects #9247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
81bab93
refactor(npm): Reduce the amount of `filterTo()`s
fviernau Oct 7, 2024
7afd6c9
refactor(node): Use `Set<File>` as the type for submodule directories
fviernau Oct 7, 2024
9aca612
refactor(npm): Use named arguments in a constructor call
fviernau Oct 7, 2024
42ca16b
chore(npm): Move a TODO comment above the function signature
fviernau Oct 7, 2024
7c516d4
refactor(npm): Replace two `!!` with `checkNotNull()`
fviernau Oct 7, 2024
510909b
fix(node): Represent workspace submodules as Projects
fviernau Oct 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
242 changes: 92 additions & 150 deletions ...e-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces-expected-output.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,42 @@ analyzer:
skip_excluded: false
result:
projects:
- id: "PNPM::pnpm-app-example:1.1.4"
sschuberth marked this conversation as resolved.
Show resolved Hide resolved
definition_file_path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/app/package.json"
authors:
- "DavidWells"
declared_licenses:
- "ISC"
declared_licenses_processed:
spdx_expression: "ISC"
vcs:
type: ""
url: ""
revision: ""
path: ""
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/app"
homepage_url: ""
scopes:
- name: "dependencies"
dependencies:
- id: "PNPM::testing-pnpm-package-a:1.0.2"
linkage: "PROJECT_DYNAMIC"
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we exclude the subtree of testing-pnpm-package-a, to not duplicate it. testing-pnpm-package-a now is a project and it has the dependencies in its scopes.

Copy link
Member

@sschuberth sschuberth Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good question. How do we handle it e.g. for Pub with references to Gradle projects, do we there also list the Gradle projects separately, @mnonnenmacher?

In any case, at least in the dependency graph representation the duplication should not matter much memory-wise, as it should be deduplicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we decide to do this, I believe it'd be good to do this in a separate commit anyway, to have it properly documented and to not increase this commit further. So, I propose we make the discussion non-blocking for this PR, and I will make a follow-up PR if needed.

Copy link
Member

@mnonnenmacher mnonnenmacher Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How do we handle it e.g. for Pub with references to Gradle projects, do we there also list the Gradle projects separately

There are two ways how Gradle dependencies can appear in Pub/Flutter projects:

  • As project dependencies, when they refer to Gradle projects in the same repository.
  • As package dependencies, when they refer to Android specific code in Pub packages.

In both cases we include transitive dependencies in the dependency tree.

I see two main reasons to include transitive dependencies for project dependencies:

  • Not all scopes are relevant, for example, in Node projects only the "dependencies" scope should be added, not the "devDependencies" scope. This is important to correctly determine excluded packages.
  • For some package managers the version resolution could yield different results if a project is not analyzed on its own but as a dependency of another project.

dependencies:
- id: "NPM::chalk:5.0.1"
- id: "NPM::is-even:1.0.0"
dependencies:
- id: "NPM::is-odd:0.1.2"
dependencies:
- id: "NPM::is-number:3.0.0"
dependencies:
- id: "NPM::kind-of:3.2.2"
dependencies:
- id: "NPM::is-buffer:1.1.6"
- id: "NPM::sax:1.2.4"
- id: "PNPM::pnpm-workspaces:1.0.1"
definition_file_path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/package.json"
authors:
Expand All @@ -50,16 +86,31 @@ analyzer:
scopes:
- name: "dependencies"
dependencies:
- id: "NPM::chalk:4.0.0"
- id: "NPM::json-stable-stringify:1.0.1"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't PNPM::pnpm-workspaces:1.0.1 also have PROJECT_DYNAMIC dependencies on the projects referred to by its workspces, i.e. on PNPM::pnpm-app-example:1.1.4, PNPM::testing-pnpm-package-a:1.0.2 and PNPM::testing-pnpm-package-b:1.0.2?

Copy link
Member

@sschuberth sschuberth Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm actually not sure about this myself. Maybe a workspace declaration does not really express a dependency relationship by itself, and thus these projects should not be listed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I planned to investigate this in my following work. In pnpm I noticed that a workspace actually can have dependencies, but workspace submodules are not shown under dependencies. To me this makes sense.
I believe a workspace is just some construct which helps with development, and also helps with using a single lockfile for all projects, but it does not really matter in terms of the actual dependencies.

I propose to not add any such dependency from the workspace to the submodules. At least not in this PR. I guess there will be a better understanding on this on my side soon anyway, so if you're fine, let's defer the topic.

Copy link
Member Author

Choose a reason for hiding this comment

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

(my planned next step is to separate the implementations (Yarn, Pnpm, Npm) and then it will be much easier to make such changes)

dependencies:
- id: "NPM::ansi-styles:4.3.0"
dependencies:
- id: "NPM::color-convert:2.0.1"
dependencies:
- id: "NPM::color-name:1.1.4"
- id: "NPM::supports-color:7.2.0"
dependencies:
- id: "NPM::has-flag:4.0.0"
- id: "NPM::jsonify:0.0.0"
- id: "PNPM::testing-pnpm-package-a:1.0.2"
definition_file_path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-a/package.json"
authors:
- "Marcel Bochtler"
declared_licenses:
- "ISC"
declared_licenses_processed:
spdx_expression: "ISC"
vcs:
type: ""
url: ""
revision: ""
path: ""
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-a"
homepage_url: ""
scopes:
- name: "dependencies"
dependencies:
- id: "NPM::chalk:5.0.1"
- id: "NPM::is-even:1.0.0"
dependencies:
Expand All @@ -70,28 +121,7 @@ analyzer:
- id: "NPM::kind-of:3.2.2"
dependencies:
- id: "NPM::is-buffer:1.1.6"
- id: "NPM::json-stable-stringify:1.0.1"
dependencies:
- id: "NPM::jsonify:0.0.0"
- id: "NPM::pnpm-workspaces:1.0.1"
dependencies:
- id: "NPM::json-stable-stringify:1.0.1"
dependencies:
- id: "NPM::jsonify:0.0.0"
- id: "NPM::sax:1.2.4"
- id: "NPM::testing-pnpm-package-a:1.0.2"
dependencies:
- id: "NPM::chalk:5.0.1"
- id: "NPM::is-even:1.0.0"
dependencies:
- id: "NPM::is-odd:0.1.2"
dependencies:
- id: "NPM::is-number:3.0.0"
dependencies:
- id: "NPM::kind-of:3.2.2"
dependencies:
- id: "NPM::is-buffer:1.1.6"
- id: "NPM::sax:1.2.4"
- name: "devDependencies"
dependencies:
- id: "NPM::require-uncached:2.0.0"
Expand All @@ -100,6 +130,38 @@ analyzer:
dependencies:
- id: "NPM::callsites:0.2.0"
- id: "NPM::resolve-from:1.0.1"
- id: "PNPM::testing-pnpm-package-b:1.0.2"
definition_file_path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-b/package.json"
authors:
- "Marcel Bochtler"
declared_licenses:
- "ISC"
declared_licenses_processed:
spdx_expression: "ISC"
vcs:
type: ""
url: ""
revision: ""
path: ""
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-b"
homepage_url: ""
scopes:
- name: "dependencies"
dependencies:
- id: "NPM::chalk:4.0.0"
dependencies:
- id: "NPM::ansi-styles:4.3.0"
dependencies:
- id: "NPM::color-convert:2.0.1"
dependencies:
- id: "NPM::color-name:1.1.4"
- id: "NPM::supports-color:7.2.0"
dependencies:
- id: "NPM::has-flag:4.0.0"
- id: "PNPM::testing-pnpm-package-c:1.0.0"
definition_file_path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/non-workspace/package-c/package.json"
authors:
Expand Down Expand Up @@ -604,66 +666,6 @@ analyzer:
url: "https://github.com/dcodeIO/long.js.git"
revision: "088e44e5e3343ef967698565678384fa474b003b"
path: ""
- id: "NPM::pnpm-app-example:1.1.4"
purl: "pkg:npm/[email protected]"
authors:
- "DavidWells"
declared_licenses:
- "ISC"
declared_licenses_processed:
spdx_expression: "ISC"
description: ""
homepage_url: ""
binary_artifact:
url: ""
hash:
value: ""
algorithm: ""
source_artifact:
url: ""
hash:
value: ""
algorithm: ""
vcs:
type: "Git"
url: "<REPLACE_URL>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/app"
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/app"
- id: "NPM::pnpm-workspaces:1.0.1"
purl: "pkg:npm/[email protected]"
authors:
- "Marcel Bochtler"
declared_licenses:
- "MIT"
declared_licenses_processed:
spdx_expression: "MIT"
description: "PNPM workspaces test"
homepage_url: ""
binary_artifact:
url: ""
hash:
value: ""
algorithm: ""
source_artifact:
url: ""
hash:
value: ""
algorithm: ""
vcs:
type: "Git"
url: "<REPLACE_URL>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces"
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces"
- id: "NPM::require-uncached:2.0.0"
purl: "pkg:npm/[email protected]"
authors:
Expand Down Expand Up @@ -785,66 +787,6 @@ analyzer:
url: "https://github.com/chalk/supports-color.git"
revision: "c5edf46896d1fc1826cb1183a60d61eecb65d749"
path: ""
- id: "NPM::testing-pnpm-package-a:1.0.2"
purl: "pkg:npm/[email protected]"
authors:
- "Marcel Bochtler"
declared_licenses:
- "ISC"
declared_licenses_processed:
spdx_expression: "ISC"
description: ""
homepage_url: ""
binary_artifact:
url: ""
hash:
value: ""
algorithm: ""
source_artifact:
url: ""
hash:
value: ""
algorithm: ""
vcs:
type: "Git"
url: "<REPLACE_URL>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-a"
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-a"
- id: "NPM::testing-pnpm-package-b:1.0.2"
purl: "pkg:npm/[email protected]"
authors:
- "Marcel Bochtler"
declared_licenses:
- "ISC"
declared_licenses_processed:
spdx_expression: "ISC"
description: ""
homepage_url: ""
binary_artifact:
url: ""
hash:
value: ""
algorithm: ""
source_artifact:
url: ""
hash:
value: ""
algorithm: ""
vcs:
type: "Git"
url: "<REPLACE_URL>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-b"
vcs_processed:
type: "Git"
url: "<REPLACE_URL_PROCESSED>"
revision: "<REPLACE_REVISION>"
path: "plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/workspaces/src/packages/package-b"
scanner: null
advisor: null
evaluator: null
Expand Down
Loading
Loading