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

feat: type safe posthog extensions #1407

Merged
merged 12 commits into from
Sep 20, 2024
Merged

Conversation

pauldambra
Copy link
Member

We keep adding more and more things directly to window
We get no type help from TS
And we're not being super unique with naming so could easily clash with someone else

Let's be a little more explicit about where and what we add to the window

Copy link

vercel bot commented Sep 9, 2024

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

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Sep 19, 2024 10:12am

Copy link

github-actions bot commented Sep 9, 2024

Size Change: +4.42 kB (+0.36%)

Total Size: 1.22 MB

Filename Size Change
dist/array.full.js 349 kB +1.32 kB (+0.38%)
dist/array.js 164 kB +786 B (+0.48%)
dist/exception-autocapture.js 10.5 kB +145 B (+1.4%)
dist/main.js 165 kB +786 B (+0.48%)
dist/module.js 164 kB +786 B (+0.48%)
dist/recorder-v2.js 111 kB +227 B (+0.21%)
dist/recorder.js 111 kB +227 B (+0.21%)
dist/surveys.js 66 kB +38 B (+0.06%)
dist/tracing-headers.js 8.36 kB +104 B (+1.26%)
ℹ️ View Unchanged
Filename Size
dist/surveys-preview.js 59.8 kB
dist/web-vitals.js 10.3 kB

compressed-size-action

Comment on lines 17 to 36
interface PosthogExtensions {
parseErrorAsProperties?: ([event, source, lineno, colno, error]: ErrorEventArgs) => ErrorProperties
errorWrappingFunctions?: {
wrapOnError: (captureFn: (props: Properties) => void) => () => void
wrapUnhandledRejection: (captureFn: (props: Properties) => void) => () => void
}
rrweb?: { record: any; version: string; rrwebVersion: string }
rrwebPlugins?: { getRecordConsolePlugin: any; getRecordNetworkPlugin?: any }
canActivateRepeatedly?: (survey: any) => boolean
webVitalsCallbacks?: {
onLCP: (metric: any) => void
onCLS: (metric: any) => void
onFCP: (metric: any) => void
onINP: (metric: any) => void
}
tracingHeadersPatchFns?: {
_patchFetch: (sessionManager: SessionIdManager) => () => void
_patchXHR: (sessionManager: any) => () => void
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

here's the secret sauce... we've added a bunch of stuff directly to window and you can't really see what without reading a bunch of files, and you don't get any typesafety at all since it's all (window as any)

we'd have to be careful here not to bring in the lazy bundles and inflate the bundle unnecessarily

but this is at least a step better

Copy link
Member

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

noice

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.

Lets go! Just a small suggestion

src/entrypoints/surveys.ts Show resolved Hide resolved
src/entrypoints/web-vitals.ts Outdated Show resolved Hide resolved
@pauldambra pauldambra added bump minor Bump minor version when this PR gets merged release alpha NB _when applied_ release an alpha version to NPM and removed release alpha NB _when applied_ release an alpha version to NPM labels Sep 16, 2024
@pauldambra pauldambra added the release alpha NB _when applied_ release an alpha version to NPM label Sep 17, 2024
@pauldambra pauldambra added release alpha NB _when applied_ release an alpha version to NPM and removed release alpha NB _when applied_ release an alpha version to NPM labels Sep 17, 2024
import { SessionIdManager } from '../sessionid'
import { ErrorEventArgs, ErrorProperties, Properties } from '../types'
import { PostHog } from '../posthog-core'
import { SurveyManager } from '../extensions/surveys'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Errrr surely this isn't what you want...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although I am surprised the bundle didn't grow much it still seems like we should never be importing from extensions...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah... the fun of offsite brain tiredness. Should have a linter maybe to save in future... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

nicer if we had interfaces instead of these anys but alreayd a lot changing in here so 🙈

Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically we could do import type and that would give you what you want but thats also at risk of getting changed and importing the actual data too...

@pauldambra pauldambra removed the release alpha NB _when applied_ release an alpha version to NPM label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged release alpha NB _when applied_ release an alpha version to NPM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants