-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
merge v3.2.2 backport into main #356
Conversation
My understanding is that the test runner was switched on the 3.x branch because you wanted to test against old versions of Node such as Node 0.4 that standard tools don't cover. However, since |
jest doesn’t support node 6, and it aggressively drops support for things. Using jest on packages is a maintenance obstacle that I’d prefer not to endure. (also, tape is just as much a “standard tool” as jest). |
Similarly, i use my custom actions on all 500+ of my projects; setup-node is largely unmaintained and only has wide usage because it was created by the GitHub actions team (who don’t seem very staffed now since it’s not AI-related) |
We could cut a v5 with a higher Jest has about 35x more usage than tape, so the dev community is definitely more familiar with it: https://npmcharts.com/compare/tape,jest?log=true&interval=30 |
Why would we want to go out of our way to exclude more users, just to use a more common test runner? Maintainer preferences matter here, not potential contributors - if it ends up being a problem (it hasn’t been one on any of my other packages), i can write the tests for contributors on their PRs. |
e7bdf65
to
bb93436
Compare
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as off-topic.
This comment was marked as off-topic.
Inspection of the code and types showed that the actual check does not need to handle all cases, but rather there is a small handful of attributes and cases to check. As such do it in the project and remove the entire need for a dep. Ref: #354
👋 @ljharb org admin here digging in after a long time away. I understand you're trying to restore backwards compatibility, and upgrading dependencies as a result. My first question is around dependencies that you personally own, like the ESLint config and GitHub actions. Rather than depending on your own stuff, are there parts that could be added to this repo instead so that other maintainers can make changes rather than being locked out of those source files in your |
@marcysutton hey! given the current outrage and harassment, i'd love if we could chat privately, and then publicly summarize the outcome afterwards. DM? |
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.
@ljharb left you some comments, mostly what we already discussed. Let me know if you have any questions!
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #356 +/- ##
======================================
Coverage ? 0.00%
======================================
Files ? 1
Lines ? 105
Branches ? 20
======================================
Hits ? 0
Misses ? 105
Partials ? 0 ☔ View full report in Codecov by Sentry. |
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 all the updates!
The merge-base changed after approval.
Followup to #354; this omits/reverts 3a89d8c, and bumps engines.node to >= 6.
It will be released as v4.0.1.