launchdarkly / react-client-sdk

LaunchDarkly Client-side SDK for React.js
Other
79 stars 67 forks source link

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

Closed hwangm closed 1 year ago

hwangm commented 1 year ago

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 commented 1 year ago

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

subvertallchris commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

This has been fixed in v2.29.3.

hwangm commented 1 year ago

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