Refactor component detection tests and update jest configuration for ESM compatibility (required to refactor to dynamically load dependency) #110
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ESM compatibility (required to refactor to dynamically load dependency)
|
Tested here... looks ok in the real world: https://github.com/vulna-felickz/my-spring-log4j-vuln-sample/actions/runs/14938235047/job/41970518830 |
felickz
requested review from
GeekMasher and
adrienpessu
and removed request for
a team
GeekMasher
suggested changes
May 11, 2025
| } | ||
| }); | ||
| }); | ||
| return manifests; | ||
| } | ||
|
|
||
| private static getDependencyScope(pkg: ComponentDetectionPackage) { | ||
| private static getDependencyScope(pkg: any) { |
There was a problem hiding this comment.
Switching to any type isn't a great idea. Please can you update to a specific type
| @@ -65,11 +64,34 @@ export default class ComponentDetection { | |||
| return parameters; | |||
| } | |||
|
|
|||
| public static async getManifestsFromResults(): Promise<Manifest[] | undefined> { | |||
| public static async getManifestsFromResults(): Promise<any[] | undefined> { | |||
| @@ -23,7 +14,7 @@ export default class ComponentDetection { | |||
| public static outputPath = './output.json'; | |||
|
|
|||
| // This is the default entry point for this class. | |||
| static async scanAndGetManifests(path: string): Promise<Manifest[] | undefined> { | |||
| static async scanAndGetManifests(path: string): Promise<any[] | undefined> { | |||
There was a problem hiding this comment.
This should return a manifest
Comment on lines
-4
to
-11
| import { | ||
| PackageCache, | ||
| BuildTarget, | ||
| Package, | ||
| Snapshot, | ||
| Manifest, | ||
| submitSnapshot | ||
| } from '@github/dependency-submission-toolkit' |
There was a problem hiding this comment.
Is there a reason to remove these imports?
| await ComponentDetection.downloadLatestRelease(); | ||
| await ComponentDetection.runComponentDetection("./test"); | ||
| // Mock the CLI output file | ||
| const mockOutput = JSON.stringify({ |
There was a problem hiding this comment.
Maybe this could be stored and ready from a file versus inline?
|
Too many nasty changes here, making a different PR for full compat |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR Summary: Node 20 & ESM/Jest Compatibility Upgrade
Fixes #104 that was due to Node 20 upgrade
Overview
This PR upgrades the project to support Node.js 20 and resolves compatibility issues with ESM-only dependencies (
octokit,@github/dependency-submission-toolkit) and Jest. The changes are more extensive than a typical Node upgrade due to the ecosystem’s shift toward ESM and the limitations of Jest/ts-jest with ESM modules.Key Changes
Dynamic ESM Imports:
octokit,@github/dependency-submission-toolkit) with dynamicawait import()calls.requireor staticimportin a CommonJS context.Jest Configuration Updates:
jest.config.jsto remove ESM-specific options and mappings that caused conflicts.Test Refactoring:
Parses CLI outputtest to decouple unit tests from the actual CLI binary and ESM runtime issues.Additional Logging:
TypeScript Adjustments:
anyor dynamic class definitions where necessary to satisfy both TypeScript and runtime requirements.Why These Changes Were Necessary
Node 20 & ESM:
Node 20 enforces stricter ESM/CJS boundaries. Many modern libraries (like
octokit) are now ESM-only, which breaks static imports in CommonJS or mixed environments.Jest/ts-jest Limitations:
Jest (and ts-jest) do not fully support ESM, especially with dynamic imports and TypeScript. This required workarounds such as dynamic imports and test mocking.
Test Reliability:
By mocking CLI output, we ensure that unit tests remain reliable and fast, and are not dependent on external binaries or network calls.
Impact