Skip to content

Add useQueryString to InitOptions#774

Merged
zikaari merged 2 commits intomasterfrom
query_param_option
Jan 27, 2023
Merged

Add useQueryString to InitOptions#774
zikaari merged 2 commits intomasterfrom
query_param_option

Conversation

@zikaari
Copy link
Copy Markdown
Contributor

@zikaari zikaari commented Jan 26, 2023

Fixes #769

New API to control the query param behaviour:

// Disable query string processing altogether
analytics.load('<WRITE_KEY>', {
  useQueryString: false
})
// OR keep it on, but enforce validation rules
analytics.load('<WRITE_KEY>', {
  useQueryString: {
    // set a pattern for anonymous id 
    aid: /([A-Z]{10})/,
    // set a pattern for user id
    uid: /([A-Z]{6})/
  }
})

@zikaari zikaari requested review from chrisradek and silesky January 26, 2023 01:01
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Jan 26, 2023

🦋 Changeset detected

Latest commit: fa3b354

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

This PR includes changesets to release 3 packages
Name Type
@segment/analytics-next Minor
@segment/analytics-core Patch
@segment/analytics-node 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

@zikaari zikaari force-pushed the query_param_option branch from 0e466fb to 06b962d Compare January 26, 2023 01:05
silesky
silesky previously approved these changes Jan 26, 2023
Copy link
Copy Markdown
Contributor

@silesky silesky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! We probably want to follow up with the docs team to get this feature documented.


async queryString(query: string): Promise<Context[]> {
async queryString(query: string): Promise<Context[] | void> {
if (this.options.useQueryString === false) {
Copy link
Copy Markdown
Contributor

@silesky silesky Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: should we return empty array instead to preserve the function signature?

Since .queryString is a public API and the function signature changed, this could be considered 'in theory' a breaking change. It would mostly affect existing typescript users who would need to update their code to handle the void case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Let me fix that

@silesky silesky dismissed their stale review January 26, 2023 05:47

realized had a potentially blocking question

@zikaari zikaari requested a review from silesky January 26, 2023 19:23
@zikaari zikaari merged commit a182c14 into master Jan 27, 2023
@zikaari zikaari deleted the query_param_option branch January 27, 2023 00:02
@github-actions github-actions bot mentioned this pull request Jan 27, 2023
@nfm
Copy link
Copy Markdown

nfm commented Feb 10, 2023

Thanks for tackling this @zikaari and @silesky 🙇

I see that #775 is now merged - do you know when the functionality is likely to be available via the JS snippet? We're loading this via "https://cdn.segment.com/analytics.js/v1/" + OUR_SEGMENT_WRITE_KEY + "/analytics.min.js".

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

Successfully merging this pull request may close these issues.

Possible to turn off the query string processing?

3 participants