-
Notifications
You must be signed in to change notification settings - Fork 129
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: Add better npm import, and script entrypoint for customizations #1550
feat: Add better npm import, and script entrypoint for customizations #1550
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size Change: -520 B (-0.02%) Total Size: 3.15 MB
ℹ️ View Unchanged
|
it's a big size bump for a convenience change... i guess they're either not pure or rollup isn't detecting them as pure... need some kind of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry should have commented here
it's a big size bump for a convenience change...
i guess they're either not pure or rollup isn't detecting them as pure...
need some kind of /@NO_SIDE_EFFECTS/ marker somewhere so that these are only available in NPM not in the bundles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the size bump tells me that this hasn't worked as expected.
I'll look into it
Apologies for the premature tagging for review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I removed the tsconfig change, and this seems to have prevented the increase in bundle size. I'll figure this part out separately.
65c11c7
to
cc0bb99
Compare
@@ -224,7 +224,7 @@ export class PostHogPersistence { | |||
const campaignParams = Info.campaignParams(this.config.custom_campaign_params) | |||
// only save campaign params if there were any | |||
if (!isEmptyObject(stripEmptyProperties(campaignParams))) { | |||
this.register(Info.campaignParams(this.config.custom_campaign_params)) | |||
this.register(campaignParams) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive-by, saw this could be smaller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lovely quality of life improvement
wanna update the before_send
docs too? #cheeky
🚀
Changes
Add a script entrypoint,
cutomizations.full.js
for adding all of the cutomizations for posthog-js.Right now this is the sampling functions and the setAllPersonPropertiesAsPropertiesForFlags function.
I've also added a central import for npm users who want to use customizations, so now they can do
import { sampleByDistinctId } from 'posthog-js/lib/src/customizations'
I tweaked tsconfig.json so tha it's notimport { sampleByDistinctId } from 'posthog-js/lib/src/customizations'
. This will mean a very small number of customers will need to update their imports when they update to this version (see https://posthog.slack.com/archives/C03P7NL6RMW/p1732554807615089?thread_ts=1730464902.575749&cid=C03P7NL6RMW )This is true of anyone who imports /lib/src, it's now just /lib(We could do this in a backwards compat way, I'm just not sure we need the extra complexity for something that was previously not officially supported)I removed the above change /lib/src/ -> /lib/ - it was causing an increase in the bundle size and I'll figure it out separately
Checklist