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

Refactor pnpm updater to handle devDependencies and optimize update logic #11304

Closed
wants to merge 6 commits into from

Conversation

kbukum1
Copy link
Contributor

@kbukum1 kbukum1 commented Jan 14, 2025

What are you trying to accomplish?

This PR refactors the pnpm updater logic to improve how it handles devDependencies and other dependencies. The changes include:

  • Raising a ToolFeatureNotSupported error for dependencies that are part of workspaces-devDependencies since we currently do not support devDependencies in workspaces.
  • Separating devDependencies and other dependencies for updates.
  • Using the --save-dev flag specifically for devDependencies.
  • Avoiding unnecessary resets of changes by skipping pnpm install when an experimental flag is enabled.

This resolves potential issues where unsupported dependencies in workspaces caused unclear errors and optimizes dependency management.

Anything you want to highlight for special attention from reviewers?

  • The decision to raise a ToolFeatureNotSupported error for workspaces-devDependencies ensures better clarity for users and aligns with the current limitations of our tool.
  • Feedback on the approach for handling unsupported dependencies and managing devDependencies with the --save-dev flag is welcome.
  • If there are better alternatives to skipping pnpm install under the experimental flag, suggestions are appreciated.

How will you know you've accomplished your goal?

  • The updater raises a clear and descriptive ToolFeatureNotSupported error for unsupported workspaces-devDependencies.
  • The new updater logic ensures that devDependencies and other dependencies are correctly updated with their respective flags.
  • The changes are behind the feature flag, enable_fix_for_pnpm_no_change_error.
  • Tests will confirm that the updates and lockfile changes behave as expected under different scenarios, including unsupported workspace dependencies.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@kbukum1 kbukum1 marked this pull request as ready for review January 14, 2025 23:30
@kbukum1 kbukum1 requested a review from a team as a code owner January 14, 2025 23:30
pavera
pavera previously approved these changes Jan 15, 2025
Copy link
Contributor

@pavera pavera left a comment

Choose a reason for hiding this comment

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

LGTM

Though it looks like we're now adding dependencies to the top level when we didn't before based on the test failure(s). You'll have to dig into that a bit to understand what should happen there, I'm not sure.

@kbukum1 kbukum1 force-pushed the kamil/fix_pnpm_no_update_dev_dependencies branch from c577d55 to 0ec20f6 Compare January 16, 2025 22:53
abdulapopoola
abdulapopoola previously approved these changes Jan 16, 2025
@kbukum1
Copy link
Contributor Author

kbukum1 commented Jan 17, 2025

Supporting pnpm workspaces is work is done. We don't a not supporting error.

@kbukum1 kbukum1 closed this Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants