Skip to content
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

build: use node 14 or higher #12

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

Kikobeats
Copy link

@Kikobeats Kikobeats commented Jan 11, 2021

The main point of this PR is

  • Avoid to maintained out of LTS node builds
  • Remove monkey patch code (String.prototype.startsWith) since is not more necessary
  • Reduce code using util.promisify

@codecov-io
Copy link

codecov-io commented Jan 11, 2021

Codecov Report

Merging #12 (7feeed7) into master (61a4c36) will decrease coverage by 1.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #12      +/-   ##
==========================================
- Coverage   86.95%   85.92%   -1.04%     
==========================================
  Files           6        6              
  Lines         138      135       -3     
==========================================
- Hits          120      116       -4     
- Misses         18       19       +1     
Impacted Files Coverage Δ
lib/get.js 81.81% <ø> (-9.10%) ⬇️
index.js 100.00% <100.00%> (ø)
lib/bin.js 78.26% <100.00%> (ø)
lib/pidtree.js 89.36% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 138eb22...210b9bd. Read the comment docs.

Copy link
Collaborator

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Kikobeats
Copy link
Author

Kikobeats commented Jan 11, 2021

not sure why windows builds are failing, probably is related to a thing not related to this PR 🤔

@Kikobeats Kikobeats changed the title build: use node 10 or superior build: use node 10 or higher Jan 11, 2021
@simonepri
Copy link
Owner

Thanks for contributing!

At the moment this DLGTM.
I think is appropriate to bump the package to a stable before dropping support for node 0.10 (altough we don't have the CI testing it, it is still supported).

Before doing this though, I need to improve the CI and find a way to test on more OS than we currently do (maybe using a mixure of vagrant and docker, suggestions welcome).

@simonepri simonepri self-requested a review January 12, 2021 10:07
@Kikobeats
Copy link
Author

@simonepri I agree, right now CI build is a problem.

What if we delay this to be shipped under 1.0.0 version?

Then both things could be done: If you need to use Node 10 or lower, use 0.x, otherwise 1.x is for you.

@simonepri
Copy link
Owner

Yes that's exactly the plan!

The PR per se looks good, it is just that some other work needs to be done before this can be merged.

@huan
Copy link
Collaborator

huan commented Mar 25, 2021

@simonepri I have merged the #14 because that improvement will not breaking anything.

However, the Node version 4/6 are removed from the CI testing by that PR.

FYI.

@simonepri simonepri force-pushed the main branch 5 times, most recently from f053814 to d3fb048 Compare June 5, 2022 18:44
Copy link

@jimmywarting jimmywarting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update versions

.github/workflows/test-macos.yml Outdated Show resolved Hide resolved
.github/workflows/test-ubuntu.yml Outdated Show resolved Hide resolved
.github/workflows/test-windows.yml Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Kikobeats Kikobeats changed the title build: use node 10 or higher build: use node 14 or higher Dec 13, 2022
@Kikobeats Kikobeats removed the request for review from simonepri December 13, 2022 08:46
@Kikobeats
Copy link
Author

Applied suggestions.

Can you take care about this PR? It's open for more than one year.

Copy link
Collaborator

@huan huan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for continue improving this PR!

Could you please bump the minor version to the next one (0.6.0 -> 0.7.0) so that we can make this change not affect the previous version users?

@Kikobeats
Copy link
Author

I prefer to don't bump numbers in a PR; That's a thing can be done after merge the PR.

It looks like nobody has the ability to move this forward, so pinging @simonepri

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants