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: Adding TypeScript build test #1957

Merged
merged 1 commit into from
Apr 16, 2023
Merged

ci: Adding TypeScript build test #1957

merged 1 commit into from
Apr 16, 2023

Conversation

wellwelwel
Copy link
Collaborator

In this PR:

  1. In package.json, add the script test:tsc-build, allowing local testing before committing new PRs:

    •  npm run test:tsc-build
  2. In .github/workflows/ci-tsc-build.yml, runs the command npm run test:tsc-build when new PRs are submitted

    • ✅ Successfully, it will exit with code 0
    • ❌ In case the tsc build command fails, it will print the build errors and exit with code 1
    • I reproduced the pattern from .github/workflows/ci-linux.yml
    • I didn't understand the main branch instead master, but I kept by default like all the other CI patterns 🥷🏻
  3. In test/tsc-build/index.ts, only imports the mysql, mysql/promise and export them

    • It allows this CI to not need to be updated because of any future changes within them
  4. In test/tsc-build/tsconfig.json, reproduces the almost options from original tsconfig.json, but:

    • Doesn't emit output files
    • Set skipLibCheck to false

Important

  • This CI has a single purpose which is to test the TypeScript build like an external module
    • It will prevent future typings incompatibilities before PRs are merged
  • The build test will not change any file

Tests

  • To test it locally, I just ran the command npm run test:tsc-build
  • To test the CI, I cloned (not forked) the master to a private repository and played between valid and invalids PRs 🥷🏻
  • After concluding all the tests, I performed the fork

@sidorares
Copy link
Owner

Thanks for the PR ( and great description, as usual )

Question: is there any benefit having it in a completely separate workflow? We could have it in ci-linux, just as an extra step ( perhaps before main unit tests )

@wellwelwel
Copy link
Collaborator Author

@sidorares Note that in this own PR checks, the CI - TypeScript Build / Node.js 18.x (pull_request) has already tested 🤹🏻‍♀️

@sidorares
Copy link
Owner

Yeah, thats one of the benefits ( better visibility, as its displayed in a top level list of checks ). Also runs in parallel

@wellwelwel
Copy link
Collaborator Author

Thanks for the PR ( and great description, as usual )

Question: is there any benefit having it in a completely separate workflow? We could have it in ci-linux, just as an extra step ( perhaps before main unit tests )

I like to separate each responsibility in their respective files.
Would you like to change it?

@sidorares
Copy link
Owner

nah, I'm fine with that. In my head it probably sits closer to unit tests but I'll probably more productive knowing immediately when its a type failure

@sidorares sidorares merged commit d6be503 into sidorares:master Apr 16, 2023
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.

None yet

2 participants