Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compartment-mapper): Dev implied by development condition only for entry package #2641

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

kriskowal
Copy link
Member

I discovered this bug while reviewing Compartment Mapper: I’ve deprecated the dev option of mapNodeModules in favor of development in the conditions set option, since generally one should want both behaviors for development mode, so one should imply the other. However, the way this is implemented causes devDependencies to be linked for every package in the presence of the development condition, and should only be in the entry package.

@kriskowal kriskowal requested a review from boneskull November 20, 2024 23:42
@kriskowal kriskowal force-pushed the kriskowal-fix-cm-lfe-test branch from f561656 to ec1380a Compare November 20, 2024 23:58
@kriskowal kriskowal force-pushed the kriskowal-fix-dev-condition-implication branch from 955846b to 1024859 Compare November 20, 2024 23:58
@kriskowal kriskowal force-pushed the kriskowal-fix-cm-lfe-test branch from ec1380a to 99a1cb5 Compare November 21, 2024 00:10
@kriskowal kriskowal force-pushed the kriskowal-fix-dev-condition-implication branch from 1024859 to b7d7b23 Compare November 21, 2024 00:10
@kriskowal kriskowal requested a review from erights November 21, 2024 01:21
Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; the logic on L883 is unnecessarily verbose imo, but not gonna block on it.

const graph = await graphPackages(
maybeRead,
canonical,
packageLocation,
conditions,
packageDescriptor,
dev,
dev || (conditions && conditions.has('development')),
Copy link
Contributor

@boneskull boneskull Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditions is never falsy

Base automatically changed from kriskowal-fix-cm-lfe-test to master November 21, 2024 20:15
@kriskowal kriskowal merged commit 27ca207 into master Nov 21, 2024
28 checks passed
@kriskowal kriskowal deleted the kriskowal-fix-dev-condition-implication branch November 21, 2024 20:16
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.

2 participants