-
Notifications
You must be signed in to change notification settings - Fork 2
feat(all): Upgrade Node.js and drop EoLs support #161
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
Conversation
b5cb101 to
6928f25
Compare
| "name": "@examples/mocha", | ||
| "private": true, | ||
| "scripts": { | ||
| "check": "yarn compile && yarn test --forbid-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.
qq, what does the "--forbid-only" flag do?
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.
@SbsCruz it causes the tests to fail if there's any it.only(..) tests. The .only method helps a lot during development, but it can inadvertently exclude tests on CI. That's why we add it to the check script 🙂
| import { RootHookObject } from "mocha"; | ||
|
|
||
| export function mochaHooks(): RootHookObject { | ||
| export function mochaHooks(): Mocha.RootHookObject { |
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 see we are calling this object from Mocha now and not importing it, but why was the import deleted?
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.
@SbsCruz remember we're not calling calling an object here, it's just the type definition. We don't need to import it because mocha exposes the whole Mocha namespace (types) for us to use 🙂
suany0805
left a comment
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.
LGTM
Upgrades the Node.js version:
Currenton .nvmrc for development.Maintenance LTS,Active LTS, andCurrent.Additionally, this PR adds some minor changes/improvements:
rimrafas a dev dependency to create a cross-platformcleanscriptcheckscript.