Skip to content

Conversation

@1000RR
Copy link

@1000RR 1000RR commented Jul 28, 2022

Summary

This PR seeks to solve 2 issues:

  • rush repo components' package.json version not in semver format fails when listed as a dependency with workspace:* in another project - [rush] rush update fails if workspace dependency version does not follow "X.Y.Z" pattern #2678
  • when running rush update and there are parsing errors in a component's package.json, pnpm does not provide stderr/stdio feedback - only exit code 1, which is not descriptive to the user. The rush update error message is ERROR: Error: The command failed with exit code 1

Details

  • The component version is manually checked for being in semver format after package.json is loaded.
  • Package.json parsing of rush repos' subprojects' validates strict JSON format - no comments, syntactically correct.

How it was tested

  • building a project containing 2 components A and B, where A imports B's workspace:* version. 2 tests cases:

    • B's package.json version is set to 1.0 (new error message shown, graceful exit)
    • B's package.json version is set to 1.0.0(runs ok, graceful exit)
  • building a project where a component's package.json is malformed JSON - with comments or extra/missing commas.

  • 👉 STEP 7: Don't forget to run "rush change": https://rushjs.io/pages/best_practices/change_logs/

@ghost
Copy link

ghost commented Jul 28, 2022

CLA assistant check
All CLA requirements met.

@1000RR 1000RR marked this pull request as draft July 28, 2022 22:13
@octogonz
Copy link
Collaborator

  • when running rush update and there are parsing errors in a component's package.json, pnpm does not provide stderr/stdio feedback - only exit code 1, which is not descriptive to the user. The rush update error message is ERROR: Error: The command failed with exit code 1

🥳 yes, let's finally fix this! 🙏

@octogonz
Copy link
Collaborator

Your new API can also fix this issue: #996

@1000RR
Copy link
Author

1000RR commented Aug 2, 2022 via email

@1000RR
Copy link
Author

1000RR commented Aug 10, 2022

@octogonz Legal has cleared the CLA - good to go.

@octogonz
Copy link
Collaborator

@1000RR if you're good with these changes, could you promote this to a non-draft PR?

@1000RR 1000RR marked this pull request as ready for review August 11, 2022 02:48
@1000RR
Copy link
Author

1000RR commented Aug 16, 2022

@octogonz Looks like it's ready to go.

@1000RR 1000RR requested a review from octogonz August 18, 2022 18:16
@1000RR
Copy link
Author

1000RR commented Aug 23, 2022

@octogonz may I ask for a final review and merge?

@octogonz
Copy link
Collaborator

@octogonz may I ask for a final review and merge?

Thanks for the reminder. And thanks for your patience -- things have been very busy lately. 😄

@octogonz octogonz merged commit 4cea9b5 into microsoft:main Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants