-
Notifications
You must be signed in to change notification settings - Fork 60
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
ci: run type tests against various TypeScript version #278
Conversation
size-limit report 📦
|
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.
Looks good! Just a few comments which are up to your discretion. 🙂
.github/workflows/ci.yml
Outdated
fail-fast: false | ||
matrix: | ||
os: [ubuntu-latest] | ||
node: [14] |
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.
Any thoughts on including Node.js 18 in this list? Node 18 is unique since it introduces Web APIs that could affect our libraries, including fetch
, Response
, and Blob
.
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.
Agree! Let's do this
.github/workflows/ci.yml
Outdated
key: ${{ matrix.os }}-node-v${{ matrix.node }}-deps-${{ hashFiles(format('{0}{1}', github.workspace, '/package-lock.json')) }} | ||
|
||
- name: Overwrite TypeScript | ||
run: npm install --save-dev typescript@${{ matrix.typescript }} |
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.
Is the --save-dev
necessary? Modifying the package-lock.json
makes the workflow less pure since it modifies the code its testing.
We could use --no-save
to only install TypeScript without modifying the repo.
(If you think changing package-lock.json
doesn't make a difference, we can keep it as is.)
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.
Brilliant! Yes
Alright
I'll merge it now, but if you have any additional comments feel free to voice them :) |
No comments — looks good to me! |
Types of changes
Description
This PR runs type tests across various version of TypeScript (4.5 through 4.9).
It fixes the few errors pointed out by those new tests.
@ts-ignore
instead of@ts-expect-error
for a specific TypeScript 4.9 issue (not sure about it, but apparently in keyword can't be used on a narrowed down expression with a nullable generic constraint microsoft/TypeScript#51501 got fixed on the upcoming 5.0 version, someone suggested to backport it to 4.9 but for now it has been ignored)This PR also refactors a bit the CI architecture to accommodate those new tests. It's a bit more complex but about as fast when not running
size-limit
and faster when running it.Resolves: #274
Checklist: