🐛 [Shopify] Defer plugin init until a checkout page view, fixing double session IDs

[lierniel]

Motivation

Shopify Custom Pixel sandboxes were creating two RUM session IDs on the same page. The storefront's
Theme Liquid snippet already runs a DD_RUM instance for every page, while the Custom Pixel's
shopifyPlugin unconditionally ran its own init() side effects (patching sandboxed iframe APIs,
wiring bindings, forcing trackViewsManually, etc.) as soon as onInit fired — with no way to know
yet whether the page was actually a checkout page. See RFC: Preventing two SDK instances from
running at the same time
(RUM-18173).

Changes

• Extended the RumPlugin.onInit contract (packages/browser-rum-core/src/domain/plugins.ts,
  preStartRum.ts) so onInit may return false to abort SDK init, or a Promise<false | void> to
  defer it. callPluginsMethod/runOnInitPlugins now run plugins' onInit in order, staying
  synchronous until a plugin returns a thenable, and time out a pending onInit after 3s (surfacing
  an error rather than hanging init forever).
• shopifyPlugin.onInit now returns a Promise that waits for the sandbox's first page_viewed
  event and only proceeds (patches iframe APIs, wires bindings, forces sandbox-specific config) once
  that event's URL matches a checkout path — so a Custom Pixel loaded on a non-checkout page no longer
  spins up a second RUM instance. initShopifyBindings's clicked/ui_extension_errored handlers are
  now gated the same way, via the shared isCheckoutPage predicate.
• Replaced the older makeShopifyRumPublicApi() init()-wrapping approach with the plugin-based
  shopifyPlugin, now exposed as DD_RUM.shopifyPlugin(...) (see updated
  packages/browser-rum-shopify/README.md).

Test instructions

• yarn test:unit --spec packages/browser-rum-core/src/domain/plugins.spec.ts --spec packages/browser-rum-core/src/boot/preStartRum.spec.ts --spec packages/browser-core/src/tools/thenable.spec.ts --spec "packages/browser-rum-shopify/**/*.spec.ts"

Checklist

• Tested locally
• Tested on staging
• Added unit tests for this change.
• Added e2e/integration tests for this change.
• Updated documentation and/or relevant AGENTS.md file

[datadog-datadog-us1-prod]

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
• Patch Coverage: 73.24%
• Overall Coverage: 77.08% (+0.12%)

_{This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 7b60dac | Docs | View more details | Give us feedback!}

[cit-pr-commenter-54b7da]

Bundles Sizes Evolution

📦 Bundle Name	Base Size	Local Size	𝚫	𝚫%	Status
Rum	181.46 KiB	182.01 KiB	+569 B	+0.31%	✅
Rum Profiler	8.43 KiB	8.43 KiB	0 B	0.00%	✅
Rum Recorder	22.31 KiB	22.31 KiB	0 B	0.00%	✅
Logs	57.52 KiB	57.52 KiB	0 B	0.00%	✅
Rum Salesforce	N/A	140.01 KiB	N/A	N/A	N/A
Rum Slim	139.47 KiB	140.01 KiB	+546 B	+0.38%	✅
Worker	22.96 KiB	22.96 KiB	0 B	0.00%	✅
Rum Shopify	N/A	203.09 KiB	N/A	N/A	N/A
Rum-shopify Profiler	N/A	8.43 KiB	N/A	N/A	N/A
Rum-shopify Recorder	N/A	3.72 KiB	N/A	N/A	N/A

[amortemousque]

💬 suggestion: If we want to decouple the logic for the onInit and onStart hooks, I would create two separate functions. We could keep it relatively simple:

/**
 * Calls each plugin's `onInit`, and returns whether the initialization should go on: `false` if any
 * plugin aborts it. Stays synchronous as long as no plugin returns a thenable.
 */
export function callPluginsOnInit(
  plugins: RumPlugin[] | undefined,
  parameter: { initConfiguration: RumInitConfiguration; publicApi: RumPublicApi }
): boolean | Promise<boolean> {
  const results = (plugins ?? []).map((plugin) => plugin.onInit?.(parameter))

  if (results.some(isThenable)) {
    return Promise.all(results.map((result) => Promise.resolve(result))).then(
      (resolvedResults) => !resolvedResults.includes(false)
    )
  }
  return !results.includes(false)
}

export function callPluginsOnRumStart(plugins: RumPlugin[] | undefined, options: OnRumStartOptions): void {
  for (const plugin of plugins ?? []) {
    plugin.onRumStart?.(options)
  }
}

[BenoitZugmeyer]

Suggestion: keep things simple, don't handle timeouts.

[lierniel]

Do you mean just ignore them completely?
My motivation to handle it is to show some meaningful error message to the plugins consumers - so they will know which plugin has failed to init and why instead of generic Timeout error message.
Also I wound't say it adds a lot of complexity to ours code - just another .catch block, but let me know if you disagree

[BenoitZugmeyer]

Yes I would ignore it completely. If you really want a timeout, move it to the salesforce plugin.

The reason I am a bit reluctant is that is the main bundle size impact, with 0 benefit for the vast majority of usages.

[BenoitZugmeyer]

can you unsubscribe instead? This isBindingInstalled makes things more complex than necessary

[lierniel]

No, there is no option to unsubscribe from shopify events unfortunately :(
Here is official API reference for analytics.subscribe method and it's returning a Promise<undefined>, not an unsubscribe function or anything I can use later to detach the listener: https://shopify.dev/docs/api/web-pixels-api/standard-api/analytics

[BenoitZugmeyer]

Ok! Then nitpick: I would introduce a function like waitFirstPageViewedEvent(analytics) to isolate the subscription logic. You could even have an async onInit function:

async onInit({ initConfiguration, publicApi }) {
      const analytics = configuration.shopifyAnalytics
      if (!analytics) {
        return false
      }
      const event = await waitFirstPageViewedEvent(analytics)
      if (!isCheckoutPage(event)) {
        return false
      }
      ...
}

[BeltranBulbarellaDD]

Question: How about if in vuePlugin.ts (and the rest of the plugins for that matter), we do:
export type VuePlugin = Omit<Required<RumPlugin>, 'onInit'> & { // vuePlugin's onInit is synchronous, unlike the generic RumPlugin interface which allows an // async onInit. Narrowing the return type here avoids @typescript-eslint/no-floating-promises // false positives on every onInit call site. onInit(options: { initConfiguration: RumInitConfiguration; publicApi: RumPublicApi }): false | void }
This way, we can declare the onInit and make in sync and not have to use the eslint ignore on each call.

[lierniel]

Ngl, it looks like more effort to us to maintain it like you suggested, because we'll have to manually narrow types almost for every plugin we have - seems like a typing issue to me. But I agree that the current approach looks bad as well, so let's discuss

[BeltranBulbarellaDD]

I agree. There are tradeoffs with both ways. Let's discuss if we can find a cleaner alternative.

[lierniel]

Do you mind if we defer the discussion for another PR? I experimented a bit and didn't find the immediate answer, so to not block the Shopify integration, let's tackle it later. I'll create the dedicated ticket for it, so wdyt?

[BeltranBulbarellaDD]

Yeah sounds fair. I'll take another look tmr morning.

[BeltranBulbarellaDD]

As discussed, maybe a new optional parameter in RumPlugin since onInit is optional could be a good approach.

[lierniel]

Created a dedicated ticket here: https://datadoghq.atlassian.net/browse/RUM-18333