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

ci: run type tests against various TypeScript version #278

Merged
merged 10 commits into from Jan 24, 2023
Merged

ci: run type tests against various TypeScript version #278

merged 10 commits into from Jan 24, 2023

Conversation

lihbr
Copy link
Member

@lihbr lihbr commented Jan 23, 2023

Types of changes

  • Chore (a non-breaking change which is related to package maintenance)
  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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.

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.

image

Resolves: #274

Checklist:

  • My change requires an update to the official documentation.
  • All TSDoc comments are up-to-date and new ones have been added where necessary.
  • All new and existing tests are passing.

@lihbr lihbr added the v7 Getting addressed or related to version 7 of the kit label Jan 23, 2023
@github-actions
Copy link

size-limit report 📦

Path Size
./dist/index.cjs 9.01 KB (0%)
./dist/index.js 6.46 KB (0%)

@lihbr lihbr marked this pull request as ready for review January 23, 2023 16:13
Copy link
Member

@angeloashmore angeloashmore left a 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 Show resolved Hide resolved
fail-fast: false
matrix:
os: [ubuntu-latest]
node: [14]
Copy link
Member

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.

Copy link
Member Author

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

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 }}
Copy link
Member

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.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Brilliant! Yes

@lihbr
Copy link
Member Author

lihbr commented Jan 24, 2023

Alright

  • Fixed the TypeScript version install with --no-save
  • Tweaked the matrix to run the suite (lint, unit, build) against Node 14, 16, 18 (and I think we'll keep that flow of running against non-EOL LTS)

I'll merge it now, but if you have any additional comments feel free to voice them :)

@lihbr lihbr merged commit c11e4ec into v7 Jan 24, 2023
@lihbr lihbr deleted the lh/v7/ci branch January 24, 2023 14:36
@angeloashmore
Copy link
Member

No comments — looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v7 Getting addressed or related to version 7 of the kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants