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.

Sign up for GitHub

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

Merged
merged 12 commits into from
Feb 27, 2024
Merged

chore: deprecate property_blacklist in favor of property_denylist #1044

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d55e2a1
chore: deprecate property_blacklist in favor of property_denylist
marandaneto Feb 27, 2024
00a0644
fix
marandaneto Feb 27, 2024
a3e6db6
fix
marandaneto Feb 27, 2024
8df94c8
add test
marandaneto Feb 27, 2024
1175883
fix
marandaneto Feb 27, 2024
151848e
fix
marandaneto Feb 27, 2024
e7d19e3
test fix
marandaneto Feb 27, 2024
91891b1
fix
marandaneto Feb 27, 2024
70ecf32
merge only once
marandaneto Feb 27, 2024
28fe605
Revert "merge only once"
marandaneto Feb 27, 2024
324d57b
add todo
marandaneto Feb 27, 2024
531fd5f
remove lock
marandaneto Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions src/__tests__/posthog-core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe('posthog core', () => {

given('config', () => ({
api_host: 'https://app.posthog.com',
property_blacklist: [],
property_denylist: [],
_onCapture: jest.fn(),
get_device_id: jest.fn().mockReturnValue('device-id'),
}))
Expand Down Expand Up @@ -95,7 +95,7 @@ describe('posthog core', () => {

it('calls update_campaign_params and update_referrer_info on sessionPersistence', () => {
given('config', () => ({
property_blacklist: [],
property_denylist: [],
_onCapture: jest.fn(),
store_google: true,
save_referrer: true,
Expand Down Expand Up @@ -151,7 +151,7 @@ describe('posthog core', () => {

given('config', () => ({
opt_out_useragent_filter: true,
property_blacklist: [],
property_denylist: [],
_onCapture: jest.fn(),
}))

Expand All @@ -172,7 +172,7 @@ describe('posthog core', () => {
it('truncates long properties', () => {
given('config', () => ({
properties_string_max_length: 1000,
property_blacklist: [],
property_denylist: [],
_onCapture: jest.fn(),
}))
given('eventProperties', () => ({
Expand All @@ -185,7 +185,7 @@ describe('posthog core', () => {
it('keeps long properties if null', () => {
given('config', () => ({
properties_string_max_length: null,
property_blacklist: [],
property_denylist: [],
_onCapture: jest.fn(),
}))
given('eventProperties', () => ({
Expand Down Expand Up @@ -219,7 +219,7 @@ describe('posthog core', () => {

it('updates persisted person properties for feature flags if $set is present', () => {
given('config', () => ({
property_blacklist: [],
property_denylist: [],
_onCapture: jest.fn(),
}))
given('eventProperties', () => ({
Expand Down Expand Up @@ -361,10 +361,10 @@ describe('posthog core', () => {

given('config', () => ({
token: 'testtoken',
property_blacklist: given.property_blacklist,
property_denylist: given.property_denylist,
sanitize_properties: given.sanitize_properties,
}))
given('property_blacklist', () => [])
given('property_denylist', () => [])

beforeEach(() => {
jest.spyOn(_info, 'properties').mockReturnValue({ $lib: 'web' })
Expand All @@ -382,8 +382,8 @@ describe('posthog core', () => {
})
})

it('respects property_blacklist', () => {
given('property_blacklist', () => ['$lib', 'persistent'])
it('respects property_denylist', () => {
given('property_denylist', () => ['$lib', 'persistent'])

expect(given.subject).toEqual({
token: 'testtoken',
Expand Down Expand Up @@ -880,7 +880,7 @@ describe('posthog core', () => {
given('config', () => ({
request_batching: true,
persistence: 'memory',
property_blacklist: [],
property_denylist: [],
_onCapture: jest.fn(),
}))

Expand Down
18 changes: 13 additions & 5 deletions src/posthog-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ export const defaultConfig = (): PostHogConfig => ({
opt_out_capturing_persistence_type: 'localStorage',
opt_out_capturing_cookie_prefix: null,
opt_in_site_apps: false,
// Deprecated, use property_denylist instead.
property_blacklist: [],
property_denylist: [],
respect_dnt: false,
sanitize_properties: null,
request_headers: {}, // { header: value, header2: value }
Expand Down Expand Up @@ -1036,13 +1038,14 @@ export class PostHog {
properties
)

const property_blacklist = this.config.property_blacklist
if (_isArray(property_blacklist)) {
_each(property_blacklist, function (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]
Copy link
Member

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

Copy link
Member Author

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.

if (_isArray(property_denylist)) {
marandaneto marked this conversation as resolved.
Show resolved Hide resolved
_each(property_denylist, function (blacklisted_prop) {
delete properties[blacklisted_prop]
})
} else {
logger.error('Invalid value for property_blacklist config: ' + property_blacklist)
logger.error('Invalid value for property_denylist config: ' + property_denylist)
}

const sanitize_properties = this.config.sanitize_properties
Expand Down Expand Up @@ -1690,9 +1693,14 @@ export class PostHog {
* // name for super properties persistent store
* persistence_name: ''
*
* // deprecated, use property_denylist instead.
* // names of properties/superproperties which should never
* // be sent with capture() calls
* // be sent with capture() calls.
* property_blacklist: []
*
* // names of properties/superproperties which should never
* // be sent with capture() calls.
* property_denylist: []
*
* // if this is true, posthog cookies will be marked as
* // secure, meaning they will only be transmitted over https
Expand Down
2 changes: 2 additions & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ export interface PostHogConfig {
opt_out_capturing_cookie_prefix: string | null
opt_in_site_apps: boolean
respect_dnt: boolean
/** @deprecated - use `property_denylist` instead */
property_blacklist: string[]
property_denylist: string[]
request_headers: { [header_name: string]: string }
on_request_error: (error: MinimalHTTPResponse) => void
/** @deprecated - use `request_headers` instead */
Expand Down
Loading