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

feat: enhance package.json finder #1286

Merged
merged 14 commits into from
Jun 17, 2024

Conversation

reey
Copy link
Contributor

@reey reey commented Jun 10, 2024

implements #1284

For assets loaded from a subdirectory of node_modules, it will pick the first package.json that actually has name and version attributes.
Added a testcase for this, which verifies this functionality for the luxon and libphonenumber-js packages.
The package.json for libphonenumber-js/max misses a version number while for luxon the name is missing.

The snapshot has quite a lot of changes:

  • For some reason the purl entries are no longer url encoded.
  • The @apollo/client/* entries have been merged into a single @apollo/client entry
  • The @babel/runtime was added as dependency

@reey reey requested a review from a team as a code owner June 10, 2024 22:03
@jkowalleck
Copy link
Member

Thanks for the implementation, @reey .

I really like the solution, but it needs to be safe and waterproof.
The thing with the "enhancement" you plan on introducing is: it opens up for a whole world of false-positives and edge cases.
Let's take the time and make sure it meets the needed edge cases. :-)

For additional tests, we need to have a

  • yarn2 setup with the edge case s in unplugged mode
  • yarn2 setup with the edge cases in non-unplugged (cached) mode
  • yarn2/pnp setup with the edge case s in unplugged mode
  • yarn2/pnp setup with the edge cases in non-unplugged (cached) mode
  • all edge cases with pnpm ... dont knwon what exists, there
  • all edge cases with "external" linked-in paths ... have fun figguring out what edge cases might exists there ...

@jkowalleck jkowalleck requested a review from a team June 10, 2024 22:20
@reey
Copy link
Contributor Author

reey commented Jun 14, 2024

@jkowalleck Thanks for the feedback.
I've done some testing with yarn, but it should not be any different as the (folder) structure still has a node_modules directory:
/home/<user>/.yarn/berry/cache/libphonenumber-js-npm-1.11.3-8309751739-10c0.zip/node_modules/libphonenumber-js/build/legacy.

The overall idea was if there would be no node_modules for some package manager, to not change the behavior from how it currently works.
In case changes are required for a specific package manager, this could still be done.

Regarding the URL encoded purls it seems that the behavior is different for windows vs linux? is this intended?

reey added 3 commits June 14, 2024 22:56
Signed-off-by: Tristan Bastian <[email protected]>
Signed-off-by: Tristan Bastian <[email protected]>
Signed-off-by: Tristan Bastian <[email protected]>
@reey reey force-pushed the enhancement-issue-1284 branch from 9683252 to 0c59504 Compare June 14, 2024 20:57
@jkowalleck
Copy link
Member

@reey, your implementation looks solid, your test beds are great!
a thing that might be missing:
add a lock file to the yarn test bed, and add a yarn set version ... before the yarn install happens for the test beds.

reey added 2 commits June 16, 2024 20:29
Signed-off-by: Tristan Bastian <[email protected]>
Signed-off-by: Tristan Bastian <[email protected]>
@reey reey force-pushed the enhancement-issue-1284 branch from 5748ee1 to 94a2286 Compare June 16, 2024 18:29
Copy link
Member

@jkowalleck jkowalleck left a comment

Choose a reason for hiding this comment

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

all looks good,
i will fix the yarn situation in another branch (#1265)
first, and will merge your feature afterward.

tsconfig.json Outdated
@@ -99,7 +99,7 @@

/* Completeness */
"skipDefaultLibCheck": false, /* Skip type checking .d.ts files that are included with TypeScript. */
"skipLibCheck": false /* Skip type checking all .d.ts files. */
"skipLibCheck": true /* Skip type checking all .d.ts files. */
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had some issues there on my windows machine..
Reverted that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this was the issue on windows:

npm run build

> @cyclonedx/[email protected] build
> run-p --aggregate-output -l build:*   

[build:node] 
[build:node] > @cyclonedx/[email protected] prebuild:node
[build:node] > node -r fs -e 'fs.rmSync("dist",{recursive:true,force:true})'
[build:node] 
[build:node] 
[build:node] > @cyclonedx/[email protected] build:node
[build:node] > tsc -b ./tsconfig.json
[build:node]
[build:node] node_modules/@types/node/globals.d.ts(72,13): error TS2403: Subsequent variable declarations must have the same type.  Variable 'AbortSignal' must be of type '{ new (): AbortSignal; prototype: AbortSignal; abort(reason?: any): AbortSignal; timeout(milliseconds: number): AbortSignal; }', but here has type '{ new (): AbortSignal; prototype: AbortSignal; }'.
ERROR: "build:node" exited with 1.

Copy link
Member

@jkowalleck jkowalleck Jun 17, 2024

Choose a reason for hiding this comment

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

ah, thanks, might add a windows CI/CT to have this coverred:
#1292

reey and others added 4 commits June 16, 2024 21:24
@jkowalleck jkowalleck changed the title enhance package.json finder feat: enhance package.json finder Jun 17, 2024
Signed-off-by: Jan Kowalleck <[email protected]>
Signed-off-by: Jan Kowalleck <[email protected]>
Signed-off-by: Jan Kowalleck <[email protected]>
@jkowalleck jkowalleck added the enhancement New feature or request label Jun 17, 2024
@jkowalleck jkowalleck merged commit bffa22b into CycloneDX:master Jun 17, 2024
13 checks passed
@jkowalleck
Copy link
Member

Thanks for the contribution,
it was released via https://github.com/CycloneDX/cyclonedx-webpack-plugin/releases/tag/v3.12.0

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

Successfully merging this pull request may close these issues.

2 participants