[FIX] Codepush: unset 'useAccessibilityLabel' and 'actionNameAttribute' properties

[marco-saia-datadog]

What does this PR do?

Fixes missing configuration properties useAccessibilityLabel and actionNameAttribute.

Overview of configuration handling

flowchart TD
    A(["**DatadogCodepushProvider**"]) --> B{"Is **DatadogProviderConfiguration** configuration? (*e.g FileBasedConfiguration*)"}
    B -- YES (EXPECTED USE-CASE) --> C["ASYNC INIT"]
    C --> E["(ASYNC) Wait for **CodePush version**"]
    E --> H["(ASYNC) Call **DatadogProvider.initialize** with full DatadogProviderConfiguration, containing credentials and version"]
    C --> F["(SYNC) Extract **partial configuration** from full configuration (**THE ISSUE IS HERE**)"]
    F --> I["(SYNC) Set up **auto-instrumentation** from **partial configuration**"]
    B -- NO (LOOSELY TYPED CONFIG)--> D["SYNC INIT"]
    D --> J["Renders **DatadogProvider** with **given configuration** (fallsback to non-codepush implementation)"]

Loading

What is the issue?

When a DatadogProviderConfiguration is passed, we manually extract a partial AutoInstrumentationConfiguration to initialize DatadogProvider in ASYNC mode (no events will be sent until DatadogCodepushProvider.initialize is explicitly called in the future, when the CodePush version is retrieved).

Here is how we do it:

const partialConfiguration = {
    trackErrors: configuration.trackErrors,
    trackResources: configuration.trackResources,
    trackInteractions: configuration.trackInteractions,
    firstPartyHosts: configuration.firstPartyHosts,
    resourceTracingSamplingRate:
        configuration.resourceTracingSamplingRate
};

There are two missing properties: useAccessibilityLabel and actionNameAttribute.

Solution

To avoid similar problems in the future, we now build the partial configuration with a custom function:

const buildPartialConfiguration = (
    configuration: DatadogProviderConfiguration
): AutoInstrumentationConfiguration => {
    const partialConfiguration: RequiredOrDiscard<AutoInstrumentationConfiguration> = {
        trackErrors: configuration.trackErrors,
        trackResources: configuration.trackResources,
        trackInteractions: configuration.trackInteractions,
        firstPartyHosts: configuration.firstPartyHosts,
        logEventMapper: configuration.logEventMapper,
        errorEventMapper: configuration.errorEventMapper,
        resourceEventMapper: configuration.resourceEventMapper,
        actionEventMapper: configuration.actionEventMapper,
        useAccessibilityLabel: configuration.useAccessibilityLabel,
        resourceTracingSamplingRate: configuration.resourceTracingSamplingRate,
        actionNameAttribute:
            configuration.actionNameAttribute ?? DISCARD_PROPERTY
    };

    return removeDiscardProperties(
        partialConfiguration
    ) as AutoInstrumentationConfiguration;
};

The custom RequiredOrDiscard type, defined as:

export const DISCARD_PROPERTY = { _dd_meta_: 'DISCARD' };
export type RequiredOrDiscard<T> = {
    [K in keyof T]-?: T[K] | typeof DISCARD_PROPERTY;
};

forces you to explicitly set each property for the AutoInstrumentationConfiguration object. We need these special types, as Required<T> would force every property to be non-optional, which does not fit our use-case, where some properties in DatadogProviderConfiguration can be undefined (e.g actionNameAttribute)

For this reason we temporarily use DISCARD_PROPERTY as the default value for undefined configuration properties, and remove them right after.

With this implementation, TSC will throw an error if there are any missing properties (or typos) in the partial configuration.

Additional Notes

• Exposed AutoInstrumentationConfiguration type from core package; it is part of DatadogProvider props, so it should have been exposed to users anyway:

type Props = PropsWithChildren<{
    /**
     * If a `DatadogProviderConfiguration` instance is passed, the SDK will start tracking errors, resources and actions and sending events.
     *
     * If a `AutoInstrumentationConfiguration` object is passed, the SDK will start tracking errors, resources and actions. To start sending events, call `DatadogProvider.initialize`.
     */
    configuration:
        | DatadogProviderConfiguration
        | AutoInstrumentationConfiguration
        | FileBasedConfiguration;
    /**
     * Callback to be run once the SDK starts sending events.
     */
    onInitialization?: () => void;
}>;

• Changed dev-dependency of core package to "@datadog/mobile-react-native": "workspace:packages/core" to be able to test changes in core while making changes to the codepush package; we can revert this if it causes problems when preparing the release

• Refactored codepush unit tests to use require instead of import, to be able to call jest.resetModules() before each test. The issue is that we have to test a logic that is not invoked when DatadogProvider has been already initialized, and its isInitialized static property is set to true. For the core package we have an internal method that allows us to reset this value for testing purposes, but it is not accessible from the codepush package.

• Added two unit tests to verify the behaviour when passing a configuration of type FileBasedConfiguration

• Minor changes to the core package to allow reliable mocking of functions for codepush unit tests

Review checklist (to be filled by reviewers)

• Feature or bugfix MUST have appropriate tests
• Make sure you discussed the feature or bugfix with the maintaining team in an Issue
• Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)
• If this PR is auto-generated, please make sure also to manually update the code related to the change

[datadog-datadog-prod-us1]

Datadog Report

Branch report: marcosaia/RUM-10172/missing-codepush-config-props
Commit report: 96f50cf
Test service: dd-sdk-reactnative

✅ 0 Failed, 663 Passed, 1 Skipped, 3.75s Total Time

[cdn34dd]

I'm having trouble understanding this, why not just make it partial ?

const partialConfiguration: Partial<AutoInstrumentationConfiguration> ?

or if you want some to be partial and others to be required, maybe doing something like this ?

const partialConfiguration: Partial<AutoInstrumentationConfiguration> & {trackInteractions: AutoInstrumentationConfiguration['trackInteractions']}

But the way we're doing the types here doesn't feel right.

[marco-saia-datadog]

The problem with Partial is that you are going to miss certain properties, because TSC won't complain.

We can't directly assign each property of DatadogProviderConfiguration to the partial configuration, because we want to skip the extra ones.

If we use Required<AutoInstrumentationConfiguration>, optional properties like actionNameAttribute will require a non-undefined value, and will make TSC complain, which is also not good.

If we use Partial<AutoInstrumentationConfiguration>, and we don't explicitly add the properties if we introduce new ones in the future, we won't catch it with TSC. Partial should be used when certain properties can be skipped, but this is not the case. Optional properties still have to be mapped using the DatadogProviderConfiguration properties.

I know this does not feel right, but I honestly could not find a good alternative to type it in a way that would force developers to map all properties.