Repository navigation
Fix/license issues - #8
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughDeleted generated Dependency Radar artifacts, added test-fixtures and fixture-management npm scripts, updated .gitignore, and refactored license discovery to propagate package paths and read licenses from package directories during aggregation. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,235,255,0.5)
participant Runner as Runner (npmLs)
participant Aggregator as Aggregator
participant Utils as Utils (license reader)
participant FS as Filesystem
end
Runner->>Aggregator: emit normalized ResolvedNode { name, version, path? }
Aggregator->>Aggregator: buildNodeMap(node) (preserve node.path into NodeInfo)
Aggregator->>Utils: readLicenseFromPackageDir(packageDir) if NodeInfo.path present
Utils->>FS: read package.json & license files under packageDir
FS-->>Utils: package metadata / license text
Utils-->>Aggregator: license info (license, licenseFile, licenseText)
Aggregator-->>Runner: aggregated NodeInfo with license and path
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 34-36: The composite npm scripts fixtures:install, fixtures:scan,
and fixtures:install:all currently omit the npm-heavy variants; update each
composite script to include the corresponding fixtures:install:npm-heavy and
fixtures:scan:npm-heavy entries so that fixtures:install runs
fixtures:install:npm-heavy, fixtures:scan runs fixtures:scan:npm-heavy, and
fixtures:install:all runs fixtures:install:npm-heavy (keeping the existing
ordering/AND chaining).
In `@test-fixtures/npm-basic/index.js`:
- Around line 1-2: The test fixture currently calls require('chalk') which fails
because Chalk v5+ is ESM-only; replace that import with one of the recommended
fixes: either downgrade Chalk to v4 in the fixture's package.json, or convert
the fixture to ESM by adding "type": "module" and changing the require to an ES
import, or (if you must keep CommonJS) replace the synchronous require('chalk')
call with a dynamic import (await import('chalk')).default before using chalk in
the console.log call so the chalk.green(...) invocation no longer throws
ERR_REQUIRE_ESM.
In `@test-fixtures/pnpm-workspace-hoisted/packages/pkg-a/index.js`:
- Line 1: The code uses require('nanoid').nanoid() which fails because nanoid v5
is ESM-only. To fix this in the module.exports assignment, replace the require
call with a dynamic import using async/await inside an async function or use
top-level await if supported. Alternatively, downgrade nanoid to version 3 for
CJS compatibility. Focus on updating the initialization of the id export in the
file to be compatible with ESM nanoid usage.
In `@test-fixtures/pnpm-workspace/packages/pkg-a/index.js`:
- Line 1: The current CommonJS export uses require('nanoid').nanoid(), which
fails with nanoid v5 (ESM-only); either pin nanoid to a CJS-compatible v3.x in
your dependencies and keep module.exports = { id: require('nanoid').nanoid() },
or change the module to use a dynamic import and export an async getter (e.g.,
replace the direct module.exports id with an async function that does const {
nanoid } = await import('nanoid'); return nanoid()) so the ESM package is loaded
correctly at runtime; locate the usage of module.exports and
require('nanoid').nanoid() to apply one of these fixes.
In `@test-fixtures/pnpm-workspace/packages/pkg-a/package.json`:
- Around line 1-9: The project currently depends on nanoid@^5.0.7 (ESM-only)
while pkg-a/index.js uses require('nanoid'); fix by changing the dependency in
package.json to a CommonJS-compatible release (e.g., "nanoid": "^3.0.0") and
reinstalling (pnpm install) so require('nanoid') in pkg-a/index.js works, or
alternatively update pkg-a/index.js to use dynamic import('nanoid')/async import
and adapt call sites if you prefer to keep nanoid@5.x.
| @@ -0,0 +1 @@ | |||
| module.exports = { id: require('nanoid').nanoid() }; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the hoisted fixture package.json for nanoid
find test-fixtures/pnpm-workspace-hoisted/packages/pkg-a -name "package.json" -exec grep -l "nanoid" {} \;
# Also check parent workspace package.json
find test-fixtures/pnpm-workspace-hoisted -maxdepth 2 -name "package.json" -exec sh -c 'echo "File: $1"; grep "nanoid" "$1" || echo "Not found"' _ {} \;Repository: JosephMaynard/dependency-radar
Length of output: 204
🏁 Script executed:
cat test-fixtures/pnpm-workspace-hoisted/packages/pkg-a/package.jsonRepository: JosephMaynard/dependency-radar
Length of output: 219
🌐 Web query:
nanoid v5 commonjs cjs support module type
💡 Result:
- Nanoid v5 is published as ESM: its
package.jsonsets"type": "module"and itsexportsdon’t provide a CommonJS (require) entry—only an ESM/default entry (plus browser/react-native mappings). [2] - CommonJS (CJS) usage options with nanoid v5:
require('nanoid')works only on newer Node setups: Node 22.12+ (works out of the box) or Node 20 with--experimental-require-module. [1]- On Node 18, use dynamic
import('nanoid')from CJS. [1]
- If you need “traditional” CJS compatibility without those Node constraints, the project recommends using
nanoid@3. [1]
Nanoid v5 is ESM-only; require() usage will fail on most Node versions.
The hoisted fixture declares nanoid@^5.0.7, which is published with "type": "module" and provides no CommonJS export. Using require('nanoid') will only work on Node 22.12+ or Node 20 with the --experimental-require-module flag. On Node 18 and earlier, this will fail at runtime. Either downgrade to nanoid@3 for traditional CJS support, or use dynamic import() with async handling.
🤖 Prompt for AI Agents
In `@test-fixtures/pnpm-workspace-hoisted/packages/pkg-a/index.js` at line 1, The
code uses require('nanoid').nanoid() which fails because nanoid v5 is ESM-only.
To fix this in the module.exports assignment, replace the require call with a
dynamic import using async/await inside an async function or use top-level await
if supported. Alternatively, downgrade nanoid to version 3 for CJS
compatibility. Focus on updating the initialization of the id export in the file
to be compatible with ESM nanoid usage.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Chores