Skip to content
This repository was archived by the owner on Apr 28, 2022. It is now read-only.

Conversation

yields
Copy link
Contributor

@yields yields commented Feb 28, 2015

we should add this to spec too cc @ianstormtaylor

@yields
Copy link
Contributor Author

yields commented Feb 28, 2015

should context.traits be merged into user.traits() too?
or why have users supply context.traits if user.traits() are persisted and we can add them to the message without them needing to attach them to every call?

yields added a commit that referenced this pull request Feb 28, 2015
normalize: keep traits in options
@yields yields merged commit eb67313 into master Feb 28, 2015
@yields yields deleted the fix/traits branch February 28, 2015 15:37
@yields
Copy link
Contributor Author

yields commented Feb 28, 2015

deploying since we need this for backwards compatibility anyway, but i think we should merge traits maybe just to keep people from needing to do .track('event', {}, { traits: {..} }).

i think it should be:

analytics.identify('id', { ... });
analytics.track('event', {}); // > implicitly sends stored traits.

@ianstormtaylor
Copy link
Contributor

Ah yes, can you add what you were thinking to the spec? Or let me know and we can get it added.

For the merging, that's a tricky one. What you say sounds right to me. But... then I think in the end we shouldn't do it, since it means we're be sending tons of extra traits with each call, and we shouldn't encourage people to be using the information in context.traits anyways? Since they should be JOINing for example in SQL instead of making things muddled. So in the end I think we should hold off on mixing in user.traits() to context.traits automatically. But I could definitely be convinced on this one... we might need to do it for sure if everything becomes server-side.

I think we might need to make another small change. Basically traits is a mistake for us here since we don't actually want to let people be able to set traits via options.traits. Similarly, we don't want to let them set userId by adding options.userId. So I think the top-level properties should only contain: anonymousId, context, integrations, timestamp. And then the others wouldn't be pulled out automatically.

Does that make sense?

@yields
Copy link
Contributor Author

yields commented Mar 3, 2015

yup, good catch, fixed in a7bd450

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants