-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fixed issue with yarn plugin hooks always failing #30390
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
Conversation
…ys fall silently + existing implementation did not do anything, because executing `getPackageVersion` always resulted in an error that was then silently suppressed Signed-off-by: eipc16 <pprzemek.312@gmail.com>
Unnecessary ChangesetsThe following package(s) are private and do not need a changeset:
Changed Packages
|
try { | ||
await getPackageVersion(toDescriptor, workspace.project.configuration); | ||
console.warn( | ||
`${toDescriptor.name} should be set to "${PROTOCOL}^" instead of "${toDescriptor.range}". Make sure this change is intentional and not a mistake.`, | ||
); | ||
} catch (_error: any) { | ||
// if there's no found version then this is likely a deprecated package | ||
// or otherwise the plugin won't be able to resolve the real version | ||
// and we should not warn them. | ||
} | ||
console.warn( | ||
`${toDescriptor.name} should be set to "${PROTOCOL}^" instead of "${toDescriptor.range}". Make sure this change is intentional and not a mistake.`, | ||
); |
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.
I was not sure what do to do with it tbh? Previous implementation did not make much sense and the descriptor for the hook should only display a warning... so now it does
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.
The changes seem to make sense to me, but that's coming from someone who's not intimately familiar with yarn plugin development. @benjdlambert looking good to 🚢 ?
packages/yarn-plugin/src/handlers/afterWorkspaceDependencyAddition.test.ts
Outdated
Show resolved
Hide resolved
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.
Looks good to me, just probably @freben's comment 🙏
…alled in tests Update packages/yarn-plugin/src/handlers/afterWorkspaceDependencyAddition.test.ts Co-authored-by: Fredrik Adelöw <freben@gmail.com> Signed-off-by: Przemek <32961666+eipc16@users.noreply.github.com>
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.
Alright, let's try it!
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Hey, I just made a Pull Request!
This should fix an issue where new dependencies where not added with backstage:^ protocol, because new yarn hooks were always failing silently. Explanation here: #30083 (comment)
✔️ Checklist
Signed-off-by
line in the message. (more info)