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

chore: Run ts on test files #1358

Merged
merged 7 commits into from
Aug 16, 2024
Merged

chore: Run ts on test files #1358

merged 7 commits into from
Aug 16, 2024

Conversation

robbie-c
Copy link
Member

@robbie-c robbie-c commented Aug 13, 2024

Changes

We're not running typescript type-checking on our test files.

In the past I've found this is pretty mixed but overall positive, on the one hand you make refactoring easier and end up catching problems you might not have caught, but on the other hand your test files end up with more type coercion which is quite noisy.

Here's my attempt at adding it to posthog-js. I created a new tsconfig.json for the test files which is less strict. If I add strict:true to the test tsconfig, there's another 309 errors left to fix - let's define this at out of scope, it's still a huge improvement in type safety without it

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Aug 15, 2024 4:17pm

@posthog-bot
Copy link
Collaborator

Hey @robbie-c! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

Copy link

github-actions bot commented Aug 13, 2024

Size Change: +232 B (+0.02%)

Total Size: 1.17 MB

Filename Size Change
dist/array.full.js 333 kB +58 B (+0.02%)
dist/array.js 154 kB +58 B (+0.04%)
dist/main.js 155 kB +58 B (+0.04%)
dist/module.js 154 kB +58 B (+0.04%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 10.4 kB
dist/recorder-v2.js 110 kB
dist/recorder.js 110 kB
dist/surveys-preview.js 59.8 kB
dist/surveys.js 66 kB
dist/tracing-headers.js 8.26 kB
dist/web-vitals.js 5.79 kB

compressed-size-action

@@ -614,11 +615,12 @@ describe('surveys', () => {
})

it('returns event based surveys that observed an event', () => {
// TODO this test fails when run in isolation
Copy link
Member Author

Choose a reason for hiding this comment

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

@Phanatic Can you take a look at this? (separate to this PR)

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

I think this is totally worth it

@robbie-c robbie-c force-pushed the chore/tun-ts-on-test-files branch from a21aa40 to f030c70 Compare August 15, 2024 16:15
@robbie-c robbie-c merged commit 0b40e5a into main Aug 16, 2024
12 checks passed
@robbie-c robbie-c deleted the chore/tun-ts-on-test-files branch August 16, 2024 07:32
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