-
Notifications
You must be signed in to change notification settings - Fork 6
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
[core] core plugins update #82
Conversation
5bfc8c0
to
c5e6849
Compare
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.
LGTM 👍
@@ -2,20 +2,56 @@ | |||
// This product includes software developed at Datadog (https://www.datadoghq.com/). | |||
// Copyright 2019-Present Datadog, Inc. | |||
|
|||
import type { UnpluginOptions } from 'unplugin'; | |||
// #imports-injection-marker |
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.
⛏️ small nit: I don't think this would be easy for a new contributor to understand that the code between these 2 markers gets replaced when the yarn cli integrity
tool runs so it might be nice to add a comment or something?
Also I could not find in the contributing.md/readme.md a mention about when to run this tool (when adding a plugin?) but maybe it's not meant to be very often
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.
Very good point, I'll update the doc(s) and add comments to give more details on this.
What and why?
Still building up #78 and trying to split it as much as possible to keep the review complexity down.
How?
This one can be summed up as: