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

refactor: Rewrite PostHogSurveys class into function #978

Closed

Conversation

fromaline
Copy link

@fromaline fromaline commented Jan 29, 2024

Changes

Rewrite PostHogSurveys class into function as proposed in issue #975
...

Checklist

@fromaline fromaline changed the title refactor: Rewrite PostHogSurveys class into function (#975) refactor: Rewrite PostHogSurveys class into function Jan 29, 2024
@pauldambra
Copy link
Member

the bundled size action can't add a comment when run on forks

but is pretty cool, in that its output says what it would have commented


Size Change: -824 B (0%)

Total Size: 765 kB

Filename Size Change
dist/array.full.js 179 kB -206 B (0%)
dist/array.js 121 kB -206 B (0%)
dist/es.js 121 kB -206 B (0%)
dist/module.js 121 kB -206 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12 kB
dist/recorder-v2.js 104 kB
dist/recorder.js 58.5 kB
dist/surveys.js 48.7 kB

compressed-size-action


so, this isn't a large enough change to make gains - or surveys isn't the place to target.

thanks for the PR, and you're welcome to keep trying, but I wouldn't say this falls into "good first issue" territory. It will probably take non-trivial changes - although I'd be happy to be proved wrong 😊

@fromaline
Copy link
Author

Thanks for the feedback. I inspected the dist locally and it seems like your current setup doesn’t minify function names either. You can double-check it yourself, but it’s definitely the case on my end.

And I suspect, that’s exactly the reason why the sizes are identical in your bundled size action.

@pauldambra
Copy link
Member

there's at least some minification going on e.g. scanning the generated code I see var Vn=function(e,t,n) but yep, there's definitely room for improvement here!

@fromaline
Copy link
Author

Got it!
It’s not an insult in any way. I just tried to point out, that your original idea of rewriting classes into functions to decrease the bundle size, doesn’t work out of the box — getSurveys and getMatchingSurveys are still in the bundled code.

Btw, do you know are there any plans to decrease the bundle size of posthog in the near future? The tool is awesome and I’ll use it anyway, but the excessive size of JS library is a bit annoying. It even causes Lighthouse warnings, like good old google analytics #966

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants