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

Updates #246

Merged
merged 1 commit into from
May 13, 2020
Merged

Updates #246

merged 1 commit into from
May 13, 2020

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented May 13, 2020

Builds on #245 .

  • Linting: Check hidden files
  • npm: Update devDeps (coveralls, eslint-plugin-node, mocha, nyc)
  • Maintenance: Add .ncurc.js to prevent peerDep. updates on running local npm-check-updates

.ncurc.js Show resolved Hide resolved
Copy link
Owner

@lo1tuma lo1tuma left a comment

Choose a reason for hiding this comment

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

Thank you. I’ve merged #245, so can you please rebase?

- npm: Update devDeps (coveralls, eslint-plugin-node, mocha, nyc)
- Maintenance: Add `.ncurc.js` to prevent peerDep. updates on running local `npm-check-updates`
@brettz9
Copy link
Contributor Author

brettz9 commented May 13, 2020

Cool, thanks! I've rebased!

@brettz9
Copy link
Contributor Author

brettz9 commented May 13, 2020

Oh--would you like an update for eslint 7 too?

@brettz9
Copy link
Contributor Author

brettz9 commented May 13, 2020

To make it easier for Github Actions, and as many other projects are dropping such support, you might want to drop end-of-lifed Node 8 support and just go to Node 10 (and add 14). I can add that to this PR if you like...

@lo1tuma
Copy link
Owner

lo1tuma commented May 13, 2020

Sounds good to me. However if we drop node 8 we should also require eslint > 7 as a peer dependency right?
I would be happy to accept such a PR.

@lo1tuma lo1tuma merged commit 7f4b574 into lo1tuma:master May 13, 2020
@brettz9
Copy link
Contributor Author

brettz9 commented May 13, 2020

No need to require it as a peer dependency as eslint < 7 still works on Node 10-14. But if you didn't want to add older eslint versions to the Github Actions matrix to be fully sure that multiple eslint versions are still working with your tests, and you wanted to ensure no consumers could accidentally use the plugin without warning if using eslint < 7, then we could bump the peerDeps to eslint 7--however you like.

@brettz9 brettz9 deleted the updates branch May 13, 2020 14:05
@brettz9 brettz9 mentioned this pull request May 13, 2020
@lo1tuma
Copy link
Owner

lo1tuma commented May 13, 2020

You are absolutely right.
But if we are already going to make a breaking change (dropping node 8) we can also combine that with dropping explicit support for older ESLint version (even if we are still compatible with older versions). WDYT?

@brettz9
Copy link
Contributor Author

brettz9 commented May 13, 2020

Sure, SGTM. And as it is only a warning, people could technically use an earlier ESLint version if they wished. I'll add it now on top of the other fixes for ESLint just added.

@brettz9
Copy link
Contributor Author

brettz9 commented May 13, 2020

Added to #247.

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

Successfully merging this pull request may close these issues.

2 participants