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.

Sign up for GitHub

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

Jump to bottom

Check types in CI #1070

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Check types in CI #1070

merged 2 commits into from
Nov 20, 2024

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Nov 13, 2024 •
edited
Loading

Description of proposed changes

Previously, the only enforcement of TypeScript was through ESLint rules from @typescript-eslint.

Related issues

Checklist

@victorlin victorlin self-assigned this Nov 13, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--zconrx November 13, 2024 21:46 Inactive
victorlin
victorlin commented Nov 13, 2024
View reviewed changes
package.json Show resolved Hide resolved
@genehack genehack mentioned this pull request Nov 13, 2024
Closed
@victorlin victorlin force-pushed the victorlin/check-types branch from 5e9722e to 4fa273c Compare November 19, 2024 23:21
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--zconrx November 19, 2024 23:21 Inactive
@victorlin victorlin force-pushed the victorlin/check-types branch from 4fa273c to 7b7fca4 Compare November 20, 2024 20:14
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--zconrx November 20, 2024 20:15 Inactive
@victorlin victorlin requested a review from genehack November 20, 2024 21:19
genehack
genehack approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@genehack genehack left a comment

Choose a reason for hiding this comment

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

non-blocking commentary.

.github/workflows/ci.yml Outdated
Comment on lines 52 to 53
- run: npm run build
- run: npm run type-check
Copy link
Contributor

Choose a reason for hiding this comment

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

if the type-checking depends on having a build, wouldn't it be better to include the build part in the type-check entry in package.json:scripts?

IOW, make this only npm run type-check here and...

package.json
@@ -17,6 +17,7 @@
"lint": "npm run lint:server",
"lint:server": "DEBUG=eslint:cli-engine npx eslint --max-warnings=0 --ext .js,.jsx .",
"lint:static-site": "cd static-site && DEBUG=eslint:cli-engine npx eslint --max-warnings=0 --ext .js,.jsx,.ts,.tsx .",
"type-check": "cd static-site && tsc",
Copy link
Contributor

Choose a reason for hiding this comment

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

... ./build.sh && cd static-site && tsc here?

@victorlin
Check types in CI
f2b2268 
Previously, the only enforcement of TypeScript was through ESLint rules
from @typescript-eslint.

Note that a build is necessary before checking to generate the
static-site/next-env.d.ts file which provides types from Next.js.
@victorlin
Remove CSS/image static import types
8ea90e5 
These are provided by Next.js in static-site/next-env.d.ts, which is
generated during build.
@victorlin victorlin force-pushed the victorlin/check-types branch from 7b7fca4 to 8ea90e5 Compare November 20, 2024 23:48
@victorlin victorlin temporarily deployed to nextstrain-s-victorlin--zconrx November 20, 2024 23:49 Inactive
@victorlin
Copy link
Member Author

@genehack good point. I've added a separate type-check:ci script for this purpose. This lets us use npm run type-check in during local dev to skip the build – it takes ~1 minute to run and is not necessary all the time.

@victorlin victorlin merged commit f560b4d into master Nov 20, 2024
6 checks passed
@victorlin victorlin deleted the victorlin/check-types branch November 20, 2024 23:53
@genehack
Copy link
Contributor

@genehack good point. I've added a separate type-check:ci script for this purpose. This lets us use npm run type-check in during local dev to skip the build – it takes ~1 minute to run and is not necessary all the time.

👍 I was hoping that ./build.sh was smart enough to only rebuild when needed. 8^/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@genehack genehack genehack approved these changes

Assignees

@victorlin victorlin

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

type error in nav/nav-logo.tsx
3 participants
@victorlin @genehack @nextstrain-bot