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

Event proxy causes undefined method push errors when calling native object methods #162

Closed
hwangm opened this issue Nov 30, 2022 · 6 comments

Comments

@hwangm
Copy link

hwangm commented Nov 30, 2022

Describe the bug
If LD is configured to send events on flag read, when application code calls native object methods such as hasOwnProperty on the flags data from the useFlags hook, an error gets thrown and subsequent events do not get sent to LD.

Uncaught TypeError: Cannot read properties of undefined (reading 'push')
    at e.getSummary (vendors~app.8706a9e4d44d6226c395.js:173:24361)
    at c.flush (vendors~app.8706a9e4d44d6226c395.js:173:36278)
    at e (vendors~app.8706a9e4d44d6226c395.js:173:36702)
    at r (vendors~app.8706a9e4d44d6226c395.js:236:31507)

To reproduce

  1. Configure LD react options without the sendEventsOnFlagRead option (or set to true) using react-client-sdk@2.27.0
  2. In a component, use the useFlags hook API to get a list of feature flags
  3. Call a native object method on the returned flags object e.g. hasOwnProperty('name') or propertyIsEnumerable('name')
  4. Observe when that application code is called, the above browser error occurs

Expected behavior
If native object methods are called on the flags object, they should be excluded from the flag read event map and no error should be thrown.

Logs
The undefined error occurs when the events are flushed to LD via the EventSummarizer (https://github.com/launchdarkly/js-sdk-common/blob/main/src/EventSummarizer.js#L63). I installed local copies of the js-client-sdk and js-sdk-common packages to add debugging around these files to see what the flag contents were (ss below):
image

I was able to identify application code that used conditionals checking against the flag name to perform some behavior:

const allFlags = useFeatureFlags();
const flag = allFlags.hasOwnProperty(name) ? allFlags[name] : null;
{...}

where useFeatureFlags is defined as:

import { useFlags } from 'launchdarkly-react-client-sdk';
const useFeatureFlags = useFlags as () => FeatureFlags;

To test whether this was the issue, I first deleted any references to hasOwnProperty on the flags object, saw that the error was gone, then added new lines calling allFlags.propertyIsEnumerable('name') to check if the error returned, resulting in the SS above. I also verified that setting the config option sendEventsOnFlagRead=false eliminates the error even if native Object methods are called.

It appears that the JS proxy introduced in 2.27.0 considers native Object methods as flag keys and will add them to the flag map for count tracking unless the config option sendEventsOnFlagRead is explicitly set to false.

SDK version
react-client-sdk@2.27.0 and higher. I verified that this bug is present in all released versions after 2.27.0.

Language version, developer tools
Node 16.18, TS 4.7.3, React 17.0.2

OS/platform
Mac OS 12.6, M1 chip

Additional context
This should be considered a breaking change if applications have code that calls native Object methods on the flags object and do not explicitly set this new configuration to false, as errors will immediately start happening after upgrade and events will stop being set to LD.

Can someone from the LD team clarify whether this feature of sending events on flag read was something that was being done by default before this change, or is a new feature as a result of introducing the JS Proxy?

@yusinto
Copy link
Contributor

yusinto commented Dec 1, 2022

Thank you for reporting this. We are investigating the issue. This has been filed internally as 178466.

@subvertallchris
Copy link

@yusinto Can you clarify: if someone upgrades to 2.27.0 and sets sendEventsOnFlagRead: false, will they lose behavior that was previously working in 2.26.0? Or is this a new feature that an existing user may not be using? We're trying to determine whether we should wait for a fix before we upgrade or if it's safe to disable the feature and move on.

@yusinto
Copy link
Contributor

yusinto commented Dec 2, 2022

@hwangm the sendEventsOnFlagRead feature is a new feature introduced in 2.27. Prior to 2.27 of the React SDK send an evaluation event for every flag on load by default.

@subvertallchris you won't lose the behavior in 2.26.0 by setting sendEventsOnFlagRead: false. You can safely upgrade to 2.27.0 and not use this new feature. Be aware though in 2.27 the sendEventsOnlyForVariation is now set to true by default, so depending on your usage, you may need to adjust your code to keep existing behavior.

Please refer to the docs for more information.

@hwangm I will investigate the error on invoking native object methods on the flags object.

@yusinto
Copy link
Contributor

yusinto commented Dec 5, 2022

We reproduced the issue and have found the cause and the corresponding fix. A new release will be published soon with the fix. Thank you all for your patience.

@yusinto
Copy link
Contributor

yusinto commented Dec 7, 2022

This has been fixed in v2.29.3.

@yusinto yusinto closed this as completed Dec 13, 2022
@hwangm
Copy link
Author

hwangm commented Jan 9, 2023

@yusinto thank you very much for the prompt updates and fix! much appreciated.

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

No branches or pull requests

3 participants