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

Update to latest dependencies #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented Jan 26, 2023

The most important change here is getting rid of new Buffer. I'll do a quick code review to explain a few of my opinionated changes -- I'm happy to change anything you like, of course.

I think I have previously signed the Strongloop CLA, although the link in CONTRIBUTING.md is now dead.

@@ -14,3 +14,4 @@ results
npm-debug.log
node_modules
coverage
.nyc_output/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the istanbul->nyc change

npm-debug.log
node_modules
coverage
.jshintrc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from .gitignore, then adding a few things that probably shouldn't be in the tarfile sent to the npm repo.

.travis.yml
.nyc_output/
test/
CHANGES.md
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be totally fine if you prefer CHANGES.md in the npm package.

.jshintrc
.travis.yml
.nyc_output/
test/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some people really like their tests in the package, but if the goal here is "small and tight" then they should be excluded IMO.

- "14"
- "16"
- "18"
- "19"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 90% sure that travis isn't going to run. Assuming it doesn't, I'd be happy to add GitHub actions to the repo.

@@ -6,8 +6,7 @@
"main": "index.js",
"scripts": {
"pretest": "jshint *.js test",
"test": "istanbul test -- _mocha -R spec",
"posttest": "test -z $npm_config_coverage || istanbul report"
"test": "nyc -r lcov _mocha -R spec"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

istanbul is deprecated, as is npm_config_coverage, which doesn't get set anymore. nyc is fast enough that it doesn't add enough overhead to the test loop to matter.

},
"engines": {
"node": ">=0.8.0"
"node": ">=14"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the breaking change, requiring a major version bump IMO. See https://github.com/nodejs/Release for justification.

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.

1 participant