-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: update readme with actionable advice #143
Conversation
README.md
Outdated
|
||
- If you like to reach out, look for issues with the <a href="https://github.com/es-tooling/ecosystem-cleanup/issues?q=is%3Aissue+is%3Aopen+label%3A%22needs+first+contact%22"><img src="https://img.shields.io/badge/needs%20first%20contact-b60205?style=flat" alt="needs first contact" style="vertical-align: middle"/></a> label and create an issue (or use some other communication channel). Paste the issue you created as answer to tagged issue here | ||
- If you want to create PRs, look for the <a href="https://github.com/es-tooling/ecosystem-cleanup/issues?q=is%3Aissue+is%3Aopen+label%3A%22accepts+prs%22"><img src="https://img.shields.io/badge/accepts%20prs-DBE1EF?style=flat" alt="accepts prs" style="vertical-align: middle"/></a> label. Paste the PR link in the tagged issue. | ||
- If you want to own the whole chain, go through: |
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.
It feels a bit intimidating that there's four steps. I wonder if we could instead provide some general guidance like "For large PRs, you may wish to open an issue in the target repository to ask if the maintainers would be amenable to such a change."
I also think we should mention something like "We generally try to avoid maintainers feeling pressured when creating issues and PRs and thus tackle these issues as individuals rather than mentioning e18e."
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.
Oh i missed that one. The 4 step process is just to showcase the process. I hope its not intimidating since I have the other 2 starting points before that (?).
I actually prefer the mention of e18e. It adds some trust and weight that this kinda is an important topic. I feel like the tone of the issue has to communicate that there is no pressure.
Co-authored-by: Ben McCann <[email protected]>
@benmccann I eased the language a bit but I am open for changes. |
@@ -7,70 +7,33 @@ debt in the JS ecosystem. | |||
Our goal is to provide a cleaner, faster and simpler dependency tree in the JS | |||
ecosystem for those who are concerned with it. | |||
|
|||
## The problem | |||
## How to help |
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.
This section kind of duplicates the next one. The only one here that isn't below is "needs alternative". I wonder if we can remove this section and potentially add that one below
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.
While you are right, that it is a bit repetitive, I am in favor of a bit more hand-holding for the start. So people are really sure, they know what they need to do. I would like to leave it but ofc I will change it if nobody likes my opinion on that topic :D
It could also be helpful to include a link to the guide: https://e18e.dev/guide/cleanup.html |
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.
Hey I really like that you're documenting processes like this! (thanks @43081j for pointing me to this PR) ❤️
A point of feedback as a maintainer who's received quite a few PRs from e18e/ecosystem-cleanup initiatives: please tell participants to always follow repository contributing guidelines. I've seen quite a few PRs that don't address accepted issues, have failing CI, don't use the right conventional commit PR titles, etc.
There are two reasons why I think this is important:
- This initiative is more than just rote dependency swap-outs. It's also about convincing the ecosystem at large that this is important enough to spend time on. If people affiliated with an initiative consistently trample over the repos they're trying to "help" then it looks like it's just mindless enthusiasm - not something thought deeply on by people who understand open source and maintenance. That devalues the initiative and biases us maintainers against it.
- Those processes are there for a reason! Oftentimes there are multiple possible replacements for a package. Projects often need time to discuss and figure out their preferred replacement strategy. Sending a PR early can be wasted work.
To be clear, I'm in favor of cleaning up the ecosystem. It's valuable work and I'm happy to see so much progress on it. I hope this comes across as constructive feedback and trying to help, not external criticism. 🙂
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.
Thanks for the feedback! I tried to incorporate it in the last commit (even stole a few of your words - hope you dont mind :D). Can you check if its better now?
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.
Lovely! I don't mind at all haha, thank you!
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.
Looking great to me! Just posting a few phrasing suggestions inline.
Co-authored-by: Josh Goldberg ✨ <[email protected]>
Co-authored-by: Josh Goldberg ✨ <[email protected]>
README.md
Outdated
code. To counter this, these packages use a custom implementation/wrapper around | ||
the native functionality instead of using the native functionality directly | ||
(since nobody can override the wrapper). | ||
The package in question needs a replacement that we can bring forward. Feel free to create one, but beware: its maintenance burden is on you! |
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'll bring this one up in discord but the tldr is I think we should say something like:
You should look for a replacement package and, if an appropriate one doesn't exist, consider making a new one. Though keep in mind new packages should be well tested and the community made aware of them before any migrations
Co-authored-by: Ben McCann <[email protected]>
Co-authored-by: Ben McCann <[email protected]>
sorry this took so long to get over the line all looks good to me now though 👍 thanks again @Fuzzyma! |
I added an explanation for the labels and what to do with each.
I also would like to include issue and pr templates that people can use when doing first contact or similar.
Please check my wording and feel free to push directly!