-
Notifications
You must be signed in to change notification settings - Fork 102
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
[office-addin-dev-settings] Update version of the commander dependency #338
[office-addin-dev-settings] Update version of the commander dependency #338
Conversation
I understand that commander is used across multiple office-addin packages, but was not sure if it was a good idea to update the dependency across all of them in one PR. I decided to limit my change to the package I was having trouble with. |
/AzurePipelines run |
Pull request contains merge conflicts. |
@TheSamsterZA Please resolve merge conflicts and submit a new iteration. Thanks, Courtney |
Thanks for bringing this to our attention. I think we should update this for all of the packages within office-addin-scripts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update this for all of the packages in office-addin-scripts.
Also, is there a related update needed for @types/commander? |
It appears that @types/command is not actually needed. It would be nice to remove that. |
One other thing, with the major version update to commander, we'll need to ensure that there are no breaking changes which cause a problem. We don't have tests for the CLI itself. The release notes for the major version updates describe breaking changes: https://github.com/tj/commander.js/tags @TCourtneyOwen We can talk offline how best to validate this. |
Closing since #339 supersedes this PR. |
Resolves #337.