-
Notifications
You must be signed in to change notification settings - Fork 9
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
refactor: project toolkit #40
Conversation
steps: | ||
- uses: actions/checkout@v4 | ||
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: ${{ matrix.node }} | ||
- run: npm i | ||
- run: npm run coverage | ||
- run: npm run 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.
prettier.
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.
Sorry what does that mean? Previously it was running tests with coverage here, now just tests without coverage. Just double checking if that's intended and/or coverage is not needed/used on CI?
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.
good point, thx. i proc coverage unconditionally now. will tidy up the scripts
```js | ||
const config = { | ||
connection: { | ||
port: 9000, | ||
port: 9000 |
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.
surprisingly, also prettier 😵 . everything in this file
|
||
module.exports = (...args) => dist.electrodeServer(...args); | ||
|
||
Object.assign(module.exports, dist); |
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.
use typescript the way typescript was intended to be used. no weird js shim to overwrite stuff
"check": "xrun xarc/check", | ||
"sample": "node test/sample/index.js", | ||
"docs": "xrun xarc/docs" | ||
"test": "jest", |
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. common, well known stuff
@@ -7,23 +7,90 @@ import { | |||
|
|||
/* eslint-disable max-len */ | |||
export type { | |||
LightMyRequestChain, InjectOptions, LightMyRequestResponse, LightMyRequestCallback, // 'light-my-request' |
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.
this whole module is pure prettier
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!
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.
Nice 👍
package.json
Outdated
"sample": "node test/sample/index.js", | ||
"docs": "xrun xarc/docs" | ||
"test": "jest", | ||
"coverage": "jest --collectCoverage", |
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.
Maybe slightly more commonly used: jest --coverage
(I had to look up that --collectCoverage
is an alias for --coverage
)
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.
package.json
Outdated
"coverage": "jest --collectCoverage", | ||
"lint": "eslint", | ||
"format": "prettier . --write", | ||
"format:check": "prettier . --write", |
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.
Did you mean --check
instead of --write
here?
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.
yes, thanks!
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.
steps: | ||
- uses: actions/checkout@v4 | ||
- uses: actions/setup-node@v4 | ||
with: | ||
node-version: ${{ matrix.node }} | ||
- run: npm i | ||
- run: npm run coverage | ||
- run: npm run 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.
Sorry what does that mean? Previously it was running tests with coverage here, now just tests without coverage. Just double checking if that's intended and/or coverage is not needed/used on CI?
strict-engines fails only in node 14.x due to ts-jest config
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
LGTM |
Problem
Every JS/TS developer knows common project toolkits (e.g. jest, prettier, etc), but only one JS/TS developer knows xarc/* toolkits.
This is a first pass at improving overall typescript stuff in electrode-fastify-server.
BREAKING CHANGE, entrypoint is nearly the same, but the untyped
module.exports(...args)
is removedSolution
Migrate to latest, common, community oriented tools