-
Notifications
You must be signed in to change notification settings - Fork 191
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
Update dependencies and test targets #221
Conversation
@patrickhulce any chance to merge this? |
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 @westy92 all of your stated goals are great 👍
just don't think we want to introduce breaking changes parts of this right now. maybe file an issue for the collection of breaking changes we'll want to batch for the next major? :)
I reverted the changes that require Node.js 10+. I'm happy to open another PR once this lands that drops Node.js 8.x support that can sit around until the project is ready for it. |
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 working on this @westy92 !
You're welcome! I addressed all the comments and it looks like all the tests passed. 🎉 Is there anything else I need to do to get this merged in? |
nope I was just waiting on the tests and then forgot about it :) thanks @westy92 ! |
Just to close the loop on this, here's my promised PR: #222 |
LTS
andstable
are14.x
and15.x
(https://nodejs.org/en/about/releases/).