[ANE-2655] Expose yarn and npm workspace packages as individual build targets#1643
Open
[ANE-2655] Expose yarn and npm workspace packages as individual build targets#1643
Conversation
jagonalez
changed the title
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWorkspace-aware build target support for Node.js projects was added. Yarn workspace members are now represented as individual build targets. New functions 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Strategy/Node.hs (1)
180-191: Targets are intentionally ignored for Pnpm and NPM analyzers.The
targetsparameter is discarded forPnpmandNPMcases. If per-workspace filtering should eventually apply to these analyzers too, consider adding a TODO comment or documenting this limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node.hs` around lines 180 - 191, The getDeps function currently discards the targets parameter for the Pnpm and NPM branches (patterns Pnpm and NPM) by calling analyzePnpmLock and analyzeNpm without passing targets; update these branches to either accept and forward per-workspace filtering to analyzePnpmLock/analyzeNpm or, if that behavior is not yet supported, add a concise TODO comment documenting this limitation (e.g. "TODO: apply FoundTargets filtering to Pnpm/NPM analyzers") next to the Pnpm and NPM clauses so future maintainers know targets were intentionally ignored and where to extend filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Strategy/Node.hs`:
- Around line 180-191: The getDeps function currently discards the targets
parameter for the Pnpm and NPM branches (patterns Pnpm and NPM) by calling
analyzePnpmLock and analyzeNpm without passing targets; update these branches to
either accept and forward per-workspace filtering to analyzePnpmLock/analyzeNpm
or, if that behavior is not yet supported, add a concise TODO comment
documenting this limitation (e.g. "TODO: apply FoundTargets filtering to
Pnpm/NPM analyzers") next to the Pnpm and NPM clauses so future maintainers know
targets were intentionally ignored and where to extend filtering.
300f95d to
d7dcc8b
Compare
Workspace members now appear as separate build targets (e.g. yarn@./:fossa-coder) instead of being merged into a single opaque target. This follows the established Maven pattern: one DiscoveredProject per workspace root with a BuildTarget per member. Users can now see individual workspace packages in `fossa list-targets` and filter them via `.fossa.yml`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jagonalez
force-pushed
the
feat/yarn-workspace-build-targets
branch
from
d7dcc8b to
3399748
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/references/strategies/languages/nodejs/npm-lockfile.md (1)
20-30: Clear documentation of workspace build targets.The documentation effectively explains the new workspace member targeting feature, including:
- Build target format and discovery (
fossa list-targets)- Filtering behavior and backward compatibility
- Version-specific limitations
The explanation is accurate and aligns with the implementation described in the PR objectives.
Optional: Minor style suggestion to improve scannability
Consider distinguishing the version limitation with a different label or separate blockquote to make it more prominent:
> them in `.fossa.yml`. When no filtering is applied (or all targets are > selected), all dependencies are included — the same as previous behavior. > Filtering to a specific subset of targets scopes the analysis to only those > members' dependencies. -> -> Note: Target-level dependency filtering is only supported for lockfile version -> 1. Version 3 lockfiles will show workspace build targets in `fossa list-targets`, -> but filtering to specific targets does not yet scope the dependency results. + +> **Limitation**: Target-level dependency filtering is only supported for lockfile +> version 1. Version 3 lockfiles will show workspace build targets in +> `fossa list-targets`, but filtering to specific targets does not yet scope the +> dependency results.This makes the version-specific limitation more discoverable for users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/references/strategies/languages/nodejs/npm-lockfile.md` around lines 20 - 30, Separate the version-specific limitation into a more prominent, standalone element to improve scannability: extract the sentence beginning "Note: Target-level dependency filtering is only supported for lockfile version 1. Version 3 lockfiles..." and render it as a distinct labeled note or blockquote (e.g., "Note" or "Version-specific limitation") so it clearly stands apart from the main explanation about fossa list-targets, `.fossa.yml`, and target filtering behavior; keep the original wording but ensure the label is visible and the block sits immediately after the paragraph describing filtering behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Strategy/Node.hs`:
- Line 41: The import qualifier "NonEmptySet" must be the full module path:
change the import to "import Data.Set.NonEmpty qualified as Data.Set.NonEmpty"
and update all call sites that currently use the alias NonEmptySet (e.g., usages
like NonEmptySet.fromList, NonEmptySet.member, NonEmptySet.insert,
NonEmptySet.toList, NonEmptySet.union) to use Data.Set.NonEmpty.fromList,
Data.Set.NonEmpty.member, etc.; apply these edits in the occurrences around the
current import and the referenced usages (near the ranges around lines 168-175
and 312).
- Around line 307-309: The function extractDepListsForTargets currently uses a
match guard on the pattern (FoundTargets targets) graph@PkgJsonGraph{..} to
branch when targetNames == allWorkspaceNames; replace that guarded equation with
a single equation using an if (or case) expression: in extractDepListsForTargets
bind the same parameters (FoundTargets targets) graph@PkgJsonGraph{..} and
implement "if targetNames == allWorkspaceNames then extractDepLists graph else
foldMap extractSingle selectedPackageJsons", keeping references to
extractDepLists, extractSingle, targetNames, allWorkspaceNames, and
selectedPackageJsons so the logic is identical but without a match guard.
In `@test/Node/NodeSpec.hs`:
- Around line 10-16: The new imports must be changed to qualified imports with
full module names and all call sites updated to use those qualifiers: import
Data.Set.NonEmpty as Data.Set.NonEmpty and replace nonEmpty →
Data.Set.NonEmpty.nonEmpty; import Data.Tagged as Data.Tagged and replace
applyTag → Data.Tagged.applyTag; qualify Graphing (e.g., Graphing) and prefix
its uses; qualify Path and Path.IO (e.g., Path, Path.IO) and update mkRelDir,
mkRelFile, (</>), getCurrentDir to Path.mkRelDir, Path.mkRelFile, Path.(</>),
Path.IO.getCurrentDir respectively; and qualify Strategy.Node (e.g.,
Strategy.Node) and update NPMLock, discover, extractDepListsForTargets,
findWorkspaceBuildTargets, getDeps to Strategy.Node.NPMLock,
Strategy.Node.discover, Strategy.Node.extractDepListsForTargets,
Strategy.Node.findWorkspaceBuildTargets, Strategy.Node.getDeps; also apply the
same qualification pattern to Test.Hspec and any other unqualified imports
mentioned in the review and update all their call sites accordingly.
---
Nitpick comments:
In `@docs/references/strategies/languages/nodejs/npm-lockfile.md`:
- Around line 20-30: Separate the version-specific limitation into a more
prominent, standalone element to improve scannability: extract the sentence
beginning "Note: Target-level dependency filtering is only supported for
lockfile version 1. Version 3 lockfiles..." and render it as a distinct labeled
note or blockquote (e.g., "Note" or "Version-specific limitation") so it clearly
stands apart from the main explanation about fossa list-targets, `.fossa.yml`,
and target filtering behavior; keep the original wording but ensure the label is
visible and the block sits immediately after the paragraph describing filtering
behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/references/strategies/languages/nodejs/yarn.md`:
- Around line 47-51: The fenced code block containing the yarn workspace lines
(the block with "yarn@./:app", "yarn@./:lib-utils", "yarn@./:lib-core") needs a
language identifier to satisfy markdownlint MD040; update the opening fence to
include a language such as text or console (for example change ``` to ```text)
so the block is identified and the linter warning is resolved.
---
Duplicate comments:
In `@src/Strategy/Node.hs`:
- Around line 305-309: The function extractDepListsForTargets uses a guard on
the FoundTargets match; replace the guard with an explicit if/then/else (or
case) to eliminate match guards: inspect targetNames and allWorkspaceNames
inside the (FoundTargets targets) branch and call extractDepLists graph when
equal, otherwise compute foldMap extractSingle selectedPackageJsons; keep
existing names (extractDepListsForTargets, FoundTargets, targetNames,
allWorkspaceNames, extractDepLists, selectedPackageJsons, extractSingle) so the
logic is identical but without a match guard.
- Line 41: Change the qualified import alias to match the module name: replace
the current import "import Data.Set.NonEmpty qualified as NonEmptySet" with
"import qualified Data.Set.NonEmpty as NonEmpty", then update every call site
that uses the alias (references like NonEmptySet.someFunction or
NonEmptySet.someValue) to use the new alias (NonEmpty.someFunction,
NonEmpty.someValue) so the qualifier matches the module path.
In `@test/Node/NodeSpec.hs`:
- Around line 10-39: Change the imports so they are imported qualified with
their full module names (e.g. use "import qualified Data.Set" not an alias) and
then update all call sites to use the full module-qualified symbols (e.g.
Data.Set.nonEmpty, Data.Set.member, Path.mkRelDir, Path.IO.getCurrentDir,
Graphing.whatever, Strategy.Node.discover, Strategy.Node.getDeps,
Strategy.Node.NPMLock, Strategy.Node.extractDepListsForTargets,
Strategy.Node.findWorkspaceBuildTargets, Strategy.Node.PackageJson.FlatDeps,
Strategy.Node.PackageJson.Manifest, Strategy.Node.PackageJson.NodePackage,
Strategy.Node.PackageJson.PackageJson, Strategy.Node.PackageJson.PkgJsonGraph,
Strategy.Node.PackageJson.PkgJsonLicense,
Strategy.Node.PackageJson.PkgJsonWorkspaces,
Strategy.Node.PackageJson.Production, Test.Effect.it', Test.Hspec.it,
Types.<symbols>) so the code uses fully qualified module names consistently.
npm workspaces can legitimately have root production dependencies, so we can't accurately determine what should be included or excluded. Restrict workspace build target support to yarn, where root deps are always workspace tooling (never published). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jagonalez
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Strategy/Node.hs (1)
302-327: Consider extracting shared helpers to reduce duplication.
mapToSetandextractSingleare duplicated betweenextractDepLists(lines 292-300) andextractDepListsForTargets(lines 319-327). This violates DRY.♻️ Suggested refactor to share helpers
+mapToSet :: Map Text Text -> Set NodePackage +mapToSet = Set.fromList . map (uncurry NodePackage) . Map.toList + +extractSingle :: Map Manifest PackageJson -> PackageJson -> FlatDeps +extractSingle lookup' PackageJson{..} = + FlatDeps + (applyTag `@Production` $ mapToSet (packageDeps `Map.union` packagePeerDeps)) + (applyTag `@Development` $ mapToSet packageDevDeps) + (Map.keysSet lookup') + extractDepLists :: PkgJsonGraph -> FlatDeps -extractDepLists PkgJsonGraph{..} = foldMap extractSingle $ Map.elems jsonLookup - where - mapToSet :: Map Text Text -> Set NodePackage - mapToSet = Set.fromList . map (uncurry NodePackage) . Map.toList - - extractSingle :: PackageJson -> FlatDeps - extractSingle PackageJson{..} = - FlatDeps - (applyTag `@Production` $ mapToSet (packageDeps `Map.union` packagePeerDeps)) - (applyTag `@Development` $ mapToSet packageDevDeps) - (Map.keysSet jsonLookup) +extractDepLists PkgJsonGraph{..} = foldMap (extractSingle jsonLookup) $ Map.elems jsonLookup extractDepListsForTargets :: FoundTargets -> PkgJsonGraph -> FlatDeps extractDepListsForTargets ProjectWithoutTargets graph = extractDepLists graph extractDepListsForTargets (FoundTargets targets) PkgJsonGraph{..} = - foldMap extractSingle selectedPackageJsons + foldMap (extractSingle jsonLookup) selectedPackageJsons where targetNames :: Set Text targetNames = Set.map unBuildTarget (NonEmptySet.toSet targets) selectedPackageJsons :: [PackageJson] selectedPackageJsons = filter (maybe False (`Set.member` targetNames) . packageName) $ Map.elems jsonLookup - - mapToSet :: Map Text Text -> Set NodePackage - mapToSet = Set.fromList . map (uncurry NodePackage) . Map.toList - - extractSingle :: PackageJson -> FlatDeps - extractSingle PackageJson{..} = - FlatDeps - (applyTag `@Production` $ mapToSet (packageDeps `Map.union` packagePeerDeps)) - (applyTag `@Development` $ mapToSet packageDevDeps) - (Map.keysSet jsonLookup)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Node.hs` around lines 302 - 327, The two functions extractDepLists and extractDepListsForTargets duplicate mapToSet and extractSingle; refactor by extracting a shared helper (e.g., mapToSet :: Map Text Text -> Set NodePackage) and a shared package-to-FlatDeps helper (e.g., packageToFlatDeps :: Map Text PackageJson -> PackageJson -> FlatDeps or packageToFlatDeps :: (Map Text PackageJson) -> PackageJson -> FlatDeps) so both extractDepLists and extractDepListsForTargets call the same helpers; update extractDepListsForTargets to reuse mapToSet and the new packageToFlatDeps helper and remove the duplicated local definitions (references: extractDepListsForTargets, extractDepLists, mapToSet, extractSingle, PkgJsonGraph, PackageJson).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Strategy/Node.hs`:
- Around line 302-327: The two functions extractDepLists and
extractDepListsForTargets duplicate mapToSet and extractSingle; refactor by
extracting a shared helper (e.g., mapToSet :: Map Text Text -> Set NodePackage)
and a shared package-to-FlatDeps helper (e.g., packageToFlatDeps :: Map Text
PackageJson -> PackageJson -> FlatDeps or packageToFlatDeps :: (Map Text
PackageJson) -> PackageJson -> FlatDeps) so both extractDepLists and
extractDepListsForTargets call the same helpers; update
extractDepListsForTargets to reuse mapToSet and the new packageToFlatDeps helper
and remove the duplicated local definitions (references:
extractDepListsForTargets, extractDepLists, mapToSet, extractSingle,
PkgJsonGraph, PackageJson).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jagonalez
changed the title
The root package.json is now a build target alongside workspace members. This ensures no dependencies are silently dropped — root deps (e.g. husky) are available on the root target. Also re-enables workspace build targets for npm lockfile projects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jagonalez
changed the title
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
yarn@./:my-monorepo,yarn@./:app,npm@./:api)fossa list-targetsand filter with.fossa.ymlChanges
src/Strategy/Node.hs:mkProjectpopulatesprojectBuildTargetsfor all workspace project types (yarn + npm).findWorkspaceBuildTargetsincludes root package name alongside workspace members.getDepspassesFoundTargetsthrough to bothanalyzeYarnandanalyzeNpmLock.extractDepListsForTargetsscopesFlatDepsto selected packages.test/Node/NodeSpec.hs: Tests forfindWorkspaceBuildTargets(root + members, single-package) andextractDepListsForTargets(all deps, scoped, root-only, all targets selected)docs/references/strategies/languages/nodejs/yarn.md: Documents workspace build target behaviordocs/references/strategies/languages/nodejs/npm-lockfile.md: Documents workspace build target behavior with lockfile version caveatChangelog.md: Unreleased entryTest plan
cabal buildsucceedscabal test unit-tests— all 8 Node tests passhlintandfourmolucleanfossa list-targetsshows root + workspace member targets for both yarn and npm workspacesfossa analyze -odep counts match released fossa 3.15.7🤖 Generated with Claude Code