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

Do some regular maintenance #59

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

Do some regular maintenance #59

wants to merge 1 commit into from

Conversation

jonkoops
Copy link
Collaborator

@jonkoops jonkoops commented Jul 5, 2024

Takes care of some overdue maintenance I've been meaning to get to, check the comments below to understand the refactoring done.

@jonkoops jonkoops requested a review from kbrabrand July 5, 2024 11:52
@jonkoops jonkoops self-assigned this Jul 5, 2024
@@ -21,16 +21,14 @@ jobs:
check-latest: true
cache: npm

- name: Cache Node modules
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Caching the node_modules introduces more overhead than just installing from the existing NPM cache, so I've removed it.

Comment on lines +27 to +32
- name: Run build
run: npm run build

- name: Run publication linter
run: npm run publint

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Building and publication linting (see below) is now done on CI as well.

@@ -1,4 +1,3 @@
node_modules
dist
coverage
.DS_Store
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file is already ignored by Git by default.

jest.config.js Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've moved the Jest configuration into a seperate file, as well as ensuring that it uses ESM all the way trough instead of compiling to CommonJS for testing.

@@ -17,7 +17,8 @@
"test": "jest",
"test:coverage": "jest --coverage",
"test:watch": "jest --watch --verbose",
"build": "tsc",
"build": "shx rm -rf dist && tsc --project tsconfig.build.json",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The build step now also removes the dist directory to ensure we don't end up with ghost files.

"build": "tsc",
"prepublishOnly": "npm run build"
"build": "shx rm -rf dist && tsc --project tsconfig.build.json",
"publint": "publint && attw . --pack --ignore-rules cjs-resolves-to-esm",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This now lints the package using publint and Are the Types Wrong? before publishing so that no mistakes are made.

"ts-jest": "^27.1.3",
"typescript": "^4.5.5"
},
"jest": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of the Jest config has been replaced as not all options are needed any longer, it has also been moved to it's own file.

"node"
],
"clearMocks": true
"@arethetypeswrong/cli": "^0.15.3",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All dependencies have been upgraded to the latest versions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've co-located the tests with the source files, this allows us to use the default Jest config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file includes some type updates now that we're extending a stricter config.

src/index.ts Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some type adjustments had to be made in order to satisfy the stricter typescript config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The configuration for the build has been split out from the base config so we can deal with build-specific options here.

@@ -1,30 +1,8 @@
{
"extends": "@tsconfig/strictest/tsconfig.json",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We now extend the strictest config from the TSConfig bases, this reduces the amount of configuration we need to keep around ourselves.

Signed-off-by: Jon Koops <jonkoops@gmail.com>
@@ -9,7 +9,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
node: [14, 16, 18]
node: [18, 20, 22]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added all currently supported Node.js versions to the matrix.

@jonkoops
Copy link
Collaborator Author

jonkoops commented Jul 5, 2024

@kbrabrand let me know if you'd like to review this one, otherwise I'll just merge it in as-is :)

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