Skip to content

Fix built-in context typing for newTracker (closes #1089) - #1090

Merged
Peter Perlepes (igneel64) merged 1 commit into
masterfrom
issue/1089-built-in-context-typing
Sep 8, 2022
Merged

Fix built-in context typing for newTracker (closes #1089)#1090
Peter Perlepes (igneel64) merged 1 commit into
masterfrom
issue/1089-built-in-context-typing

Conversation

@igneel64

@igneel64 Peter Perlepes (igneel64) commented Aug 24, 2022

Copy link
Copy Markdown
Contributor

Fix context option typing for newTracker

Allow for passing either webPage or session options as expexted.
Adds the BuiltInContexts type.

ctx

Notes

  • Add the RequireAtLeastOne helper for types. (Could prove useful in the future for the whole project) Rightfully stolen from @azure
  • Some typos

closes #1089

@snowplowcla Snowplow CLA bot (snowplowcla) added the cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. label Aug 24, 2022
@igneel64 Peter Perlepes (igneel64) added type:defect Bugs or weaknesses. The issue has to contain steps to reproduce. and removed cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. labels Aug 24, 2022
@igneel64
Peter Perlepes (igneel64) force-pushed the issue/1089-built-in-context-typing branch 3 times, most recently from 3829f47 to bee3289 Compare August 24, 2022 17:47

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is really cool, I didn't know about the trick!

I'm just wondering if it is a breaking change? Technically it might break the build for some users in case they set an empty dictionary in their apps? If so, strictly speaking we should add this to the next major release instead of a minor. (btw, don't forget to change the base branch to a release branch before merging)

*/
connectionTimeout?: number;
/**
* Condifugration for Anonymous Tracking

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some of the typos are quite funny :D

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Have you never tried to Condifugrate your trackers before?

@igneel64
Peter Perlepes (igneel64) force-pushed the issue/1089-built-in-context-typing branch from bee3289 to bf405c2 Compare August 29, 2022 08:58
@igneel64

Copy link
Copy Markdown
Contributor Author

This is really cool, I didn't know about the trick!

I'm just wondering if it is a breaking change? Technically it might break the build for some users in case they set an empty dictionary in their apps? If so, strictly speaking we should add this to the next major release instead of a minor. (btw, don't forget to change the base branch to a release branch before merging)

Accounted for people that could have added {} as the input on contexts. This makes it a non breaking change.
(Will add a PR for next major release to remove it completely)

@igneel64 Peter Perlepes (igneel64) changed the title Fix built-in context typing for newTracker (fix #1089) Fix built-in context typing for newTracker (closes #1089) Sep 5, 2022
@igneel64
Peter Perlepes (igneel64) merged commit e6a47a0 into master Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:defect Bugs or weaknesses. The issue has to contain steps to reproduce.

Development

Successfully merging this pull request may close these issues.

Typing issue on tracker configuration for built-in contexts

4 participants