Skip to content

chore(ci): type check entire project in ci#1538

Merged
marvinhagemeister merged 3 commits into
freshframework:mainfrom
deer:type_check_project_ci
Jul 31, 2023
Merged

chore(ci): type check entire project in ci#1538
marvinhagemeister merged 3 commits into
freshframework:mainfrom
deer:type_check_project_ci

Conversation

@deer

@deer deer commented Jul 28, 2023

Copy link
Copy Markdown
Contributor

The logical continuation of #1536 and #1537. Now that the project is clean (after those are merged), we should prevent any such errors from sneaking in again in the future.

A question here: why are individual things type checked in the CI? Specifically:

      - name: Type check init script
        run: deno check --remote init.ts

      - name: Type check website
        run: deno check main.ts dev.ts
        working-directory: www/

      - name: Type check demo
        run: deno check --remote main.ts dev.ts
        working-directory: demo

      - name: Type check tests/fixture
        run: deno check main.ts dev.ts
        working-directory: tests/fixture/

      - name: Type check tests/fixture_error
        run: deno check main.ts dev.ts
        working-directory: tests/fixture_error/

Opening as a draft because:

  1. this will fail due to the other PRs not being merged yet
  2. open question regarding duplicate type checking
  3. should I pull the check into the matrix like the other type checks?

I do agree that deno check --remote init.ts should remain separate, but the other four seem superfluous if we decide to check the whole project.

@deer

deer commented Jul 28, 2023

Copy link
Copy Markdown
Contributor Author

Thoughts on removing the extra type checks? Thoughts on why we type check individual items in every matrix run? I picked startsWith(matrix.os, 'ubuntu') && matrix.deno == 'canary' for type checking since that's what Lino did for typos, but maybe we should do the full type check in every run.

This is fortunately passing now.

@deer deer marked this pull request as ready for review July 28, 2023 09:02

@iuioiua iuioiua left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestions:

  • Create a new check:types task in deno.json and use demo task check:types in CI
  • Also check .tsx files

WDYT?

@deer

deer commented Jul 31, 2023

Copy link
Copy Markdown
Contributor Author

Thanks, I think that's very similar to what I've done in #1454. So maybe I'll pull the changes to deno.json out of 1454 and leave that focused on init.ts.

So here in this PR I'd have...

  • check:types which does deno check **/*.ts && deno check **/*.tsx
  • change the existing run: deno check **/*.ts to run: deno task check:types

Any thoughts on my confusion around the existing type checks in CI? And how it runs for all matrix variations?

@deer deer force-pushed the type_check_project_ci branch from cbb4b9c to 76a366f Compare July 31, 2023 06:13

@iuioiua iuioiua left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! It's OK to run the type check on all platforms + versions.

@deer

deer commented Jul 31, 2023

Copy link
Copy Markdown
Contributor Author

LGTM! It's OK to run the type check on all platforms + versions.

Ok, I can remove the condition then. Should I also remove the other type checks, now that we're checking everything at once?

@iuioiua

iuioiua commented Jul 31, 2023

Copy link
Copy Markdown
Contributor

Sure 🙂

@deer

deer commented Jul 31, 2023

Copy link
Copy Markdown
Contributor Author

error: Import 'https://deno.land/std@0.173.0/path/posix.ts' failed: 500 Internal Server Error
at https://deno.land/std@0.173.0/path/mod.ts:26:25

Please rerun CI 🤞

@marvinhagemeister marvinhagemeister left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for adding this 👍

@marvinhagemeister marvinhagemeister merged commit 5fa6a67 into freshframework:main Jul 31, 2023
@deer deer deleted the type_check_project_ci branch July 31, 2023 12:36
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.

3 participants