search

janbiasi / rollup-plugin-sbom

Create SBOMs in CycloneDX format for your Vite or Rollup projects with ease
MIT License
6 stars 2 forks source link

fix: plugin no longer crashes in rollup only scenarios #68

Closed xenobytezero closed 5 months ago

xenobytezero commented 5 months ago

Tasks

The plugin currently crashes when used in a Rollup only context.

The registerTools() function does a require.resolve at the top level, which will fail if the requested package does not exist. This commit reworks the registerTools function to wrap it in a try/catch.

While doing this, we also found that the code will not resolve packages like Rollup correctly. So this has also been fixed.

A require.resolve on rollup (tested with 4.14.3) will resolve to the rollup/dist/rollup.js path. The current code will then try and find a package.json at dist/package.json which will fail and so not include the package.

I attempted to add a rollup fixture for the testing purposes, but the current structure of the tests means that the code will traverse all the way up to the workspace root from the fixture folders and find both rollup and vite, regardless of which fixture is being used. I have the fixture locally but have not included it in this PR to reduce complexity.

xenobytezero commented 5 months ago

Hey @xenobytezero! Thanks a lot for the various fixes and adjustments, appreciate it 😃

You've mentioned that you prepared a fixture for the test, but it will always resolve the root packages, right? We could disable hoisting in general as we only have one single production package in the "monorepo" itself.

I had thought about this, but disabling the hoisting wont on it's own help.

In a Rollup scenario, Vite is still as a direct dependency in the root workspace, and the request.resolve() code will continue to travel all the way up regardless. The code could maybe be changed to use the getCorrespondingPackageFromModuleId code which could be set to a very specific traversal limit for the tests, but that seems a bit brittle and doesnt actually work for all scenarios (Vite would need a max traversal of 1, Rollup needs 2 because of the mentioned export map problem). It's doable, but I feel there has to be a better solution out there. If I come up with anything I'll let you know.

xenobytezero commented 5 months ago

Apologies for the extra commits, had to do some author fixing.

xenobytezero commented 5 months ago

I also added some additional filtering to the moduleParsed analysis to ignore Rollup virtual modules (where the IDs start with \0) as I noticed a lot of resolutions that were traversing all the way up the directory tree because they couldn't resolve a package.json due to the strange character at the start of the path. All the tests continue to pass.

janbiasi commented 5 months ago

@all-contributors please add @xenobytezero for bug reports and code

allcontributors[bot] commented 5 months ago

@janbiasi

I've put up a pull request to add @xenobytezero! :tada:

janbiasi commented 5 months ago

@xenobytezero thanks a lot for your efforts on the bugs! I'm now fine merging it even without tests as it seems quite hard to implemented (did some fiddling tonight but couldn't find a way to declare sufficient test cases neither). I'd be fine moving forward and merging your PR - is everything included from your side?

I will also merge #70 after this PR got merged.

xenobytezero commented 5 months ago

Yep, that's now everything from my side. Happy to have the PR merged.

github-actions[bot] commented 5 months ago

:tada: This PR is included in version 1.1.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: