Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename the analytics API #19

Open
nelsonleite opened this issue Nov 23, 2021 · 1 comment
Open

Rename the analytics API #19

nelsonleite opened this issue Nov 23, 2021 · 1 comment
Assignees
Labels
📦 analytics Relative to the `@farfetch/blackout-analytics` package postponed If for some reason this issue lost priority and was postponed to be implemented/fixed in the future. 🚨 BREAKING CHANGE Something that introduces a breaking API change status: accepted It's clear what the subject of the issue is about, and what the resolution should be type: feature New feature

Comments

@nelsonleite
Copy link
Contributor

Is your proposal related to a problem?

To make the analytics API more consistent and easier to understand.

Describe the solution you'd like

Lifecycle events

The Integration class method setConsent should be renamed to onSetConsent to be consistent with onSetUser method.
The loadIntegration event type should be dropped in favour of the onSetUser event that should be called automatically by analytics if the user was set in analytics through the analytics.setUser method.

Analytics

  • Change analytics.user method to analytics.getCurrentUserData. CurrentUser was chosen to clear some ambiguity that analytics.user could bring as, without looking at the documentation of its only argument, we could think that this function accepts a user id and retrieves the instance of the user associated with that id. Also, this name makes it clear it will only get the data that was set and not a user instance that can manipulate the user data.
  • Change analytics.consent method to analytics.getConsentData. Same reasoning as the one used in analytics.getCurrentUserData except that there was no need to use 'current' in the name as there is no ambiguity in that case.
  • Change analytics.context method to analytics.getContextData (same reasoning as explained before)
  • Change analytics.integration method to analytics.getIntegrationInstance to make it clear that it will return the fully instantiated instance of the integration associated with its name argument.
  • There can also be some ambiguity regarding the names of the analytics.setUser and analytics.setConsent methods as we could imply by their names that the previous set values are completely overridden by the values passed on the call to these methods. However, that is not the case as they are only merged with the previous values. For example, if a call to setUser method is made with 10 as the userId and { isGuest: true, email: [email protected] } as its properties and then another call is made to setUser with 20 as the userId and { isGuest: false }, it could be expected that the email property would be dropped, which is not the case. However, other analytics libraries that have the same nomenclature (ga/firebase set methods for example) works the same way, so it is ok to keep using the same names, while also documenting this behaviour and indicating alternative methods to clear all the data. For setUser we have the anonymize method but for setConsent there is nothing available that clears all data, so it might make sense to add one as well.

Describe alternatives you've considered

About the loadIntegration event type, there are some integrations that access it to get other data set in analytics. However, none of them is looking at the loadIntegration event type specifically. They are either accessing the context or consent variables, which probably means that the loadData parameter is used not as an event but as a shortcut to get some analytics values, so we could ditch the event property altogether from the loadData parameter as it is not an event per see.

Regarding the changes for analytics.user, analytics.consent and analytics.integration method names, maybe there is no need to have such verbose method names. Since we'll not change what data they return, it will continue to be clear for the developers what these methods are for, and we could simplify the naming a bit by just adding the prefix get{MethodName}, to match the opposite methods we have that have the prefix set{MethodName} for setting those values.

Additional information

Decision log 19 Feb:

  • Lifecycle events must have a signature like @onIntegrationInit both for the type and for event (and export these constants for reusability);
  • Getters and setters follow the same rule: get{MethodName} and set{MethodName}. (addIntegration will continue the same, but the getter will be changed to getIntegration);
@nelsonleite nelsonleite added status: accepted It's clear what the subject of the issue is about, and what the resolution should be type: feature New feature 📦 analytics Relative to the `@farfetch/blackout-analytics` package 🚨 BREAKING CHANGE Something that introduces a breaking API change labels Nov 23, 2021
@nelsonleite nelsonleite added this to the 💎 Blackout 2.0 milestone Jan 20, 2022
@ruifcnunes ruifcnunes added the postponed If for some reason this issue lost priority and was postponed to be implemented/fixed in the future. label May 3, 2022
@ruifcnunes
Copy link
Contributor

It was decided within the Comms team that this implementation will be postponed to a future version, after the current next is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 analytics Relative to the `@farfetch/blackout-analytics` package postponed If for some reason this issue lost priority and was postponed to be implemented/fixed in the future. 🚨 BREAKING CHANGE Something that introduces a breaking API change status: accepted It's clear what the subject of the issue is about, and what the resolution should be type: feature New feature
Development

No branches or pull requests

3 participants