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

feat: Stab to integrate current connector and extract identity storage #5

Closed
wants to merge 2 commits into from

Conversation

qingzhuozhen
Copy link
Contributor

@qingzhuozhen qingzhuozhen commented Feb 13, 2022

Summary

This is a stab effort to collect feedbacks on how we incorporate the current connector, and extract identity storage, and also how our observe plugin integrates together.

  • Use apiKey to maintain the instance. Since we are not setting up getInstance now, we need some other values to maintain instance. Temporarily use apiKey.
  • Add IdentityStorage in that package, and storage only load values when initialize, then use a general listener as others.
  • Analytics settings will apply once receive the identity update
  • Further user id and device id update will trigger to IdentityStore, then came back via listener.

The earlier commits are based on #4. Please ignore when review this change.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

@qingzhuozhen qingzhuozhen changed the title Stab to integrate current connector and extract identity storage feat: Stab to integrate current connector and extract identity storage Feb 13, 2022
Comment on lines +120 to +122
val safeListeners = synchronized(listenersLock) {
listeners.toSet()
}

Choose a reason for hiding this comment

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

curious what this does and what happens if not all listeners are returned?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the toSet is just to reduce the duplicate listeners


init {
require(configuration.isValid()) { "invalid configuration" }
timeline = Timeline().also { it.amplitude = this }
storage = configuration.storageProvider.getStorage(this)
logger = configuration.loggerProvider.getLogger(this)
idContainer = IdContainer.getInstance(configuration.apiKey, IMIdentityStorageProvider())
idContainer.identityStore.addIdentityListener(AnalyticsIdentityListener(store))

Choose a reason for hiding this comment

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

question, does this make amplitude client listen for changes happening outside? while this emits changes happening inside amplitude client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, this connects to identity store, then we can have support plugins for changes happening outside

@qingzhuozhen
Copy link
Contributor Author

Closing this one and moving the change in #8

@qingzhuozhen qingzhuozhen deleted the identity-stab-draft branch March 26, 2022 19:04
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.

2 participants