Fix wrong utm parsing by making context.page.search the source of truth for query params.

[silesky]

Fixes a utm-parameter parsing bug where overridden page.search properties would not be reflected in the context.campaign object

analytics.page(undefined, undefined, {search: "?utm_source=123&utm_content=content" )
analytics.track("foo", {url: "....", search: "?utm_source=123&utm_content=content" )

// should result in a context.campaign of:
{ source: 123, content: 'content'}

This fix is part 2 --
See: #838

[changeset-bot]

🦋 Changeset detected

Latest commit: e7e3d75

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@segment/analytics-next	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[silesky]

@pooyaj might have some insight here?

The presence of this useless guard against missing context makes me think that this logic was written before Page Enrichment, and this is tech debt. This points to a broader refactor : It would be simpler / make sense if the page enrichment plugin was was also responsible for this logic:

  if (query && !ctx.campaign) {
    ctx.campaign = utm(query)
  }

If the current bugfix is safe, then moving the logic up to page enrichment (a "before" plugin) altogether should be equally safe / rational. Let me know if I'm missing something!

[pooyaj]

Yeah, I think this is mostly legacy behavior from AJS classic where the UTM params were injected in the Segment integration rather than in AJS core by accessing window.location.search directly. I think this change is great. But moving the UTM logic to the page enrichment plugin has some consequences (i.e., UTM params will be sent as a part of the context to all destinations ). I'm not sure if that would be ok or not tho.

[silesky]

Thanks, that makes sense. I guess we've decided that, while we pass the query string, we don't want to pass ctx.campaign to all destinations.

1. Do you have any insight into this line (i.e. can we just delete this?):

json.context = json.context ?? json.options ?? {}

1. .context should always be defined (see page enrichment plugin). if it gets deleted by a bad plugin, that should be a runtime error.
2. even if .context somehow wasn't defined (i.e. deleted by some custom plugin), json.options as a fallback? why would we ever want to set event.context to:

  integrations?:
  timestamp?:
  context?: // (event.context.context?) lol
  anonymousId?:
  userId?: 
  traits?: // this is the only one that makes sense

[pooyaj]

Yeah, I think this is mostly legacy behavior from AJS classic where the UTM params were injected in the Segment integration rather than in AJS core by accessing window.location.search directly. I think this change is great. But moving the UTM logic to the page enrichment plugin has some consequences (i.e., UTM params will be sent as a part of the context to all destinations ). I'm not sure if that would be ok or not tho.

[danieljackins]

Changes look good to me, might be worth testing it out on one of the affected customer pages as an override

[silesky]

Changes look good to me, might be worth testing it out on one of the affected customer pages as an override

Yep, I've tested it. FTR, this is not the end of the fix -- this just enables a bandaid where upset customers can fix staleness by using manually passing page props (e.g. {url: "foo"}) in their page calls.

The underlying problem is that our buffered queue has never captured page information at call time -- assuming, wrongly that when analytics loads, the info will be the same.