-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
chore!: Normalize repository, dropping node <10.13 support #16
Conversation
405da19
to
2a37966
Compare
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 looking into this @sttk! I had a few things.
package.json
Outdated
"test": "mocha --async-only", | ||
"cover": "nyc --reporter=lcov --reporter=text-summary npm test", | ||
"coveralls": "npm run cover && istanbul-coveralls", | ||
"cover": "nyc mocha --async-only", |
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.
I think this is wrong. We move the nyc into the "test" script, right?
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.
Got it. I'll modify it.
package.json
Outdated
"expect": "^27.3.1", | ||
"mocha": "^8.4.0", | ||
"nyc": "^15.1.0", | ||
"v8flags": "^3.2.0" |
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.
We'll need to release this and update to the next major
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.
I just released v8flags v4.0.0
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.
I got it.
package.json
Outdated
"respawn": "node test/bin/respawner --harmony test", | ||
"nospawn": "node test/bin/respawner test" |
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.
How are these used?
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.
I think they aren't used, too. I'll remove them.
Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
Co-authored-by: Blaine Bublitz <blaine.bublitz@gmail.com>
@phated After using For compatibility of the cache file, shouldn't we keep this separator underscore? And Node.js>=v10 also accepts both dash and underscore as v8 option's separator. |
Sorry, we should use dash and underscore separately depending on |
@sttk this will be a semver major version increase, so we can stop supporting the snake_case flags. Even though node still supports these flags, we only supported them because old versions of node did not support hyphens. |
@phated I got it. I'll remove the codes which replace dashes in v8 flags to underscores. |
@phated I've modified what you pointed out. Please review them. |
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.
Excellent work! Thanks @sttk 🎊
@phated Thank you for merging! |
This PR updates this project:
I separated testing to
npm test
andnpm run cover
because one test case does not work on nyc.Closes #15