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

moves or adds tsc --noEmit from test to build phase #2597

Merged
merged 15 commits into from
Apr 18, 2024

Conversation

martin-lysk
Copy link
Contributor

@martin-lysk martin-lysk commented Apr 16, 2024

We missed type checks in some packages - most paraglide packages and manage. this lead to type not been caught by the ci.

This pr moves all tsc --noEmit calls from test to build phase and adds them where it was missing to ensure we have type safeness in all packages.

It was moved to the test phase in the past because of performance issues. I suggest to move it to build phase - since test is to slow during development. If we see problems with this approach we can change it later on.

This needs #2596 and #2595 land in main first to be green

From the chat:

@inlang/team martin and me discovered today that not all packages were doing type checks in ci leading to missed issues that went to main. in order to fix this martin added a type check to all build commands of packages where the build did not naturally do type checks by using tsc compiler here: #2562

before merging this, i would like to get team alignment this is the way to go or if we should instead have typechecking be part of the lint or test run or a new run that would be before build.

running before build has some advantages like being able to stop on type issues faster without being slowed down by parallel builds, but added complexity of having additional step and also not always checking types on local builds might lead to missing issues locally. i am neutral on this but just want everyone to be on the same page so please just confirm or comment if you find a good reason to do one or the other way

(in future everyone please make sure to add typechecking for everything using esbuild, vite, rollup or webpack as these wont error on type issues)

Copy link

changeset-bot bot commented Apr 16, 2024

⚠️ No Changeset found

Latest commit: 43d8101

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@martin-lysk
Copy link
Contributor Author

@LorisSigrist paraglide doesn't seem to like the new build proccess - can i pass this PR over to you to have a look?

@samuelstroschein samuelstroschein temporarily deployed to add-typecheck-to-build - inlang-manage PR #2597 April 17, 2024 14:47 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to add-typecheck-to-build - inlang-website PR #2597 April 17, 2024 15:32 — with Render Destroyed
@samuelstroschein samuelstroschein temporarily deployed to add-typecheck-to-build - fink-editor PR #2597 April 17, 2024 15:41 — with Render Destroyed
@janfjohannes
Copy link
Contributor

@martin-lysk @LorisSigrist i noticed that some types are only working after they have been build. this means that having typechecks in the same ci step as build might fail in unexpected ways. maybe this is related to some of the issues you are seeing, not sure. if yes we should move type checks out of build into its own pipeline step and run after build.

@LorisSigrist LorisSigrist marked this pull request as ready for review April 18, 2024 07:06
@LorisSigrist LorisSigrist merged commit 94a6535 into main Apr 18, 2024
5 checks passed
@LorisSigrist LorisSigrist deleted the add-typecheck-to-build branch April 18, 2024 07:06
@github-actions github-actions bot locked and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants