Skip to content

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.

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.

"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