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

pnpm9 optional packages detection #1180

Merged
merged 6 commits into from
Jun 19, 2024
Merged

pnpm9 optional packages detection #1180

merged 6 commits into from
Jun 19, 2024

Conversation

prabhu
Copy link
Collaborator

@prabhu prabhu commented Jun 19, 2024

Fixes #1163
Fixes #1170

@pcbowers, could you kindly test this PR and let me know how it looks?

Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>
Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>
@prabhu prabhu added enhancement New feature or request Ready for QA pnpm labels Jun 19, 2024
@prabhu prabhu requested a review from setchy as a code owner June 19, 2024 16:45
@pcbowers
Copy link
Contributor

It partially worked. It reduced the included components from 1366 to 430. However, it should be 12 (that's the total number included when using pnpm v8).Here's the package.json I used to test it:

{
  "name": "pnpm9",
  "version": "1.0.0",
  "packageManager": "pnpm@9.4.0",
  "scripts": {
    "cdxgen": "cdxgen --required-only --no-babel --type ts --print"
  },
  "private": true,
  "dependencies": {
    "@angular/animations": "17.3.11",
    "@angular/common": "17.3.11",
    "@angular/compiler": "17.3.11",
    "@angular/core": "17.3.11",
    "@angular/forms": "17.3.11",
    "@angular/platform-browser": "17.3.11",
    "@angular/platform-browser-dynamic": "17.3.11",
    "@angular/router": "17.3.11",
    "rxjs": "7.8.1",
    "ts-cacheable": "1.0.10",
    "tslib": "2.6.3",
    "zone.js": "0.14.4"
  },
  "devDependencies": {
    "@angular-builders/jest": "17.0.3",
    "@angular-devkit/build-angular": "17.3.8",
    "@angular-eslint/builder": "17.5.2",
    "@angular-eslint/eslint-plugin": "17.5.2",
    "@angular-eslint/eslint-plugin-template": "17.5.2",
    "@angular-eslint/schematics": "17.5.2",
    "@angular-eslint/template-parser": "17.5.2",
    "@angular/cli": "17.3.8",
    "@angular/compiler-cli": "17.3.11",
    "@cyclonedx/cdxgen": "10.6.2",
    "@ngneat/spectator": "19.0.0",
    "@types/jest": "29.5.12",
    "@types/node": "20.14.5",
    "@typescript-eslint/eslint-plugin": "7.13.1",
    "@typescript-eslint/parser": "7.13.1",
    "eslint": "8.57.0",
    "eslint-plugin-prefer-arrow": "1.2.3",
    "eslint-plugin-unused-imports": "4.0.0",
    "jest": "29.7.0",
    "jest-junit": "16.0.0",
    "jest-preset-angular": "14.1.0",
    "ng-mocks": "14.13.0",
    "ts-jest": "29.1.5",
    "ts-node": "10.9.2",
    "typescript": "5.4.5"
  }
}

It worked with this simpler package.json:

{
  "name": "pnpm9",
  "version": "1.0.0",
  "packageManager": "pnpm@9.4.0",
  "scripts": {
    "cdxgen": "cdxgen --required-only --no-babel --type ts --print"
  },
  "dependencies": {
    "react": "18.3.1"
  },
  "devDependencies": {
    "@cyclonedx/cdxgen": "10.6.2",
    "esbuild": "^0.21.5",
    "typescript": "5.4.5"
  }
}

I'm not sure what's different about the output on the more realistic example as compared to the simpler one, but I do know that it's still not quite working. Progress though!

(To test, I ran pnpm link on both while checked out to this branch. Then I compared to the output after unlinking. Then I downgraded pnpm to v8.15.8 and compared to that output.)

@prabhu
Copy link
Collaborator Author

prabhu commented Jun 19, 2024

@pcbowers, 12 means we are including just the direct dependencies without any of the transitive ones. That doesn't sound correct. Thoughts?

Found a limitation. Let me improve the logic a bit more. Thank you so much for the new example!

@prabhu prabhu marked this pull request as draft June 19, 2024 19:04
Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>
@prabhu
Copy link
Collaborator Author

prabhu commented Jun 19, 2024

@pcbowers with the latest commit, I am getting double digit count. I will compare against pnpm list --prod to see if there is something obvious. Could you attach some more test cases?

Update: Works for the test case attached and correctly reports 12.

Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>
@prabhu prabhu marked this pull request as ready for review June 19, 2024 19:37
Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>
Signed-off-by: Prabhu Subramanian <prabhu@appthreat.com>
@prabhu prabhu merged commit 392dca2 into master Jun 19, 2024
25 checks passed
@prabhu prabhu deleted the feature/pnpm9-optional branch June 19, 2024 21:09
@pcbowers
Copy link
Contributor

@prabhu Was able to take a look at this today and noticed that the simpler package.json is now broken with some of the more recent changes:

{
  "name": "pnpm9",
  "version": "1.0.0",
  "packageManager": "pnpm@9.4.0",
  "scripts": {
    "cdxgen": "cdxgen --required-only --no-babel --type ts --print"
  },
  "dependencies": {
    "react": "18.3.1"
  },
  "devDependencies": {
    "@cyclonedx/cdxgen": "10.6.2",
    "esbuild": "^0.21.5",
    "typescript": "5.4.5"
  }
}

Looking into it, it looks like react depends on loose-envify which depends on js-tokens. However, since @cyclonedx/cdxgen also depends on js-tokens which is a dev dependency, it excludes it. Looking at your code now to see if there's a simple fix and will try to put in a PR, but will also convert this into an issue too. This was probably not caught because when you link @cyclonedx/cdxgen, any of its underlying dependencies are not included.

@prabhu
Copy link
Collaborator Author

prabhu commented Jun 20, 2024

@pcbowers must be this commit

Below is the output I get which looks fine to me.

✦ ❯ pnpm list --prod
Legend: production dependency, optional only, dev only

pnpm9@1.0.0 /Volumes/Work/sandbox/issue-pnpm-react

dependencies:
react 18.3.1

Work/sandbox/issue-pnpm-react is 📦 v1.0.0 via  v22.2.0
❯ node /Volumes/Work/CycloneDX/cdxgen/bin/cdxgen.js -o bom.json . -t js --required-only -p
╔════════════════════════════════╗
║        Dependency Tree         ║
║  Generated with ♥  by cdxgen   ║
╟────────────────────────────────╢
║ pkg:npm/react@18.3.1           ║
║ └── pkg:npm/loose-envify@1.4.0 ║
╚════════════════════════════════╝

╔═══════════════════════════╤═════════════════════════════════════╤═══════════════════════════╤═════════════════╗
║ Group                     │ Name                                │                   Version │ Scope           ║
╟───────────────────────────┼─────────────────────────────────────┼───────────────────────────┼─────────────────╢
║                           │ loose-envify                        │                     1.4.0 │                 ║
╟───────────────────────────┼─────────────────────────────────────┼───────────────────────────┼─────────────────╢
║                           │ react                               │                    18.3.1 │                 ║
╚═══════════════════════════╧═════════════════════════════════════╧═══════════════════════════╧═════════════════╝
BOM includes 2 components and 2 dependencies

@pcbowers
Copy link
Contributor

Actually, the output we would want here should include js-tokens since that is a sub dependency of loose-envify. Just put in a PR!

@prabhu
Copy link
Collaborator Author

prabhu commented Jun 20, 2024

@pcbowers any reasons why pnpm is not showing js-tokens? Can you ask them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated Bom Does Not Match PNPM Lockfile Structure PNPM Lockfile Version 9.0 Breaks --required-only Option
2 participants