-
Notifications
You must be signed in to change notification settings - Fork 131
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: deprecate property_blacklist in favor of property_denylist #1044
Conversation
Hey @marandaneto! 👋 |
Size Change: +869 B (0%) Total Size: 860 kB
ℹ️ View Unchanged
|
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.
a little nit-picking about where/when we handle the config but otherwise
src/posthog-core.ts
Outdated
_each(property_blacklist, function (blacklisted_prop) { | ||
delete properties[blacklisted_prop] | ||
// since property_blacklist is deprecated in favor of property_denylist, we merge both of them here | ||
const property_denylist = [...this.config.property_blacklist, ...this.config.property_denylist] |
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.
also feels like we should merge this once, i think maybe we have to do it in init and in set_config
that way internally the SDK only has a denylist, and existing config gets copied into it whenever we evaluate the config
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.
discussed offline, and added a TODO for now, it requires a bit of test refactoring to make it work since the override option always replaces the set_config call.
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.
Apart from what looks like a typo 👍
This reverts commit 70ecf32.
Changes
Let's make our config more inclusive.
Checklist