Skip to content

fix(js): honor optional dependency overrides - #590

Merged
rowkav09 merged 1 commit into
mainfrom
fix/js-optional-dependency-override
Oct 1, 2026
Merged

rowkav09 merged 1 commit into
mainfrom
fix/js-optional-dependency-override

Conversation

@rowkav09

@rowkav09 rowkav09 commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Closes #589.

Honor npm's documented optionalDependencies precedence over duplicate dependencies entries. Remove stale required runtime row/constraint while retaining effective optional dependency and unrelated required entries.

Fail-first regression, then all470 JS adapter tests+lint green.

@rowkav09

rowkav09 commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

APPROVE at e14d230

When a name is in both dependencies and optionalDependencies, the runtime row is now dropped and only the optional one (with its constraint) is emitted. That matches npm, where optionalDependencies override dependencies of the same name. devDependencies and peerDependencies keep their own rows, as before. A non-object optionalDependencies value is ignored safely. The new test checks the shared name comes out once as optional "2" and the other runtime dependency is untouched. JS/TS adapter suite on the head merged with main: 470/470.

CI: not checked by me; the lane merges on APPROVE plus green CI.

Reviewed as rowkav09, the same account as the author, so this is not an independent GitHub approval.

@rowkav09
rowkav09 force-pushed the fix/js-optional-dependency-override branch from e14d230 to 02868d0 Compare October 1, 2026 21:16
@rowkav09

rowkav09 commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

Re-ack at 02868d0: APPROVE.

The only change since e14d230 is extracting the optional-name set into optionalDependencyNames(). The behaviour is the same: a dependencies entry whose name is also in optionalDependencies is skipped, dev and peer rows are unchanged, and a non-object optionalDependencies gives an empty set. JS/TS adapter suite on the head merged with main passes. Reviewed as rowkav09, the same account as the author, so this is not an independent GitHub approval.

@rowkav09
rowkav09 merged commit e092372 into main Oct 1, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JS optionalDependencies override is emitted as a required runtime dependency too

1 participant