Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions integrations/segmentio/HISTORY.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
4.4.0 / 2020-02-05
==================

* Removes the `bundledConfigIds` _setting_ and instead generates `bundledConfigIds` using the intersection
of `bundled` destinations and the new `maybeBundledConfigIds` setting. `bundledConfigIds` and `unbundledConfigIds`
is still appended to event metadata. This change is meant to ensure that `bundledConfigIds` only includes the
subset of destinations that is generated at runtime for the `bundled` array.
* Fixes the use of `const` from a previous release.

4.3.0 / 2020-01-13
==================

Expand Down
30 changes: 26 additions & 4 deletions integrations/segmentio/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ var utm = require('@segment/utm-params');
var uuid = require('@lukeed/uuid').v4;
var Queue = require('@segment/localstorage-retry');

const json = JSON;
var json = JSON;
Copy link
Contributor Author

@gpsamson gpsamson Feb 5, 2021

Choose a reason for hiding this comment

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

This was unintentionally switched to const in #553.


/**
* Cookie options
Expand Down Expand Up @@ -64,7 +64,9 @@ var Segment = (exports = module.exports = integration('Segment.io')
.option('saveCrossDomainIdInLocalStorage', true)
.option('retryQueue', true)
.option('addBundledMetadata', false)
.option('unbundledIntegrations', []));
.option('unbundledIntegrations', []))
.option('unbundledConfigIds', [])
.option('maybeBundledConfigIds', {});
Comment on lines +68 to +69
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we were to conditionally add these arrays in ajs-renderer we would still default to empty values here. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I agree.


/**
* Get the store.
Expand Down Expand Up @@ -315,11 +317,31 @@ Segment.prototype.normalize = function(message) {
}
if (this.options.addBundledMetadata) {
var bundled = keys(this.analytics.Integrations);
var maybeBundledConfigIds = this.options.maybeBundledConfigIds

// Generate a list of bundled config IDs using the intersection of
// bundled destination names and maybe bundled config IDs.
var bundledConfigIds = []
Copy link
Contributor

@mericsson mericsson Feb 8, 2021

Choose a reason for hiding this comment

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

I think var bundledConfigIds = [] should be defined outside of his conditional for clarity. This works currently because of use of var vs let/const.

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 don't think so -- msg._metadata.bundledConfigIds (L343) is defined within the conditional. I'm also mimicking the scope of bundled.

for (var i = 0; i < bundled.length; i++) {
var name = bundled[i]
if (!maybeBundledConfigIds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be clearer if this conditional was outside of the loop since it does not depend on anything in bundled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd have to refactor and wrap the for-loop in a conditional. I can go either way but was trying to avoid nested conditionals.

if (maybeBundledConfigIds) {
  for (var i = 0; i < bundled.length; i++) { ... }
}

break
}
if (!maybeBundledConfigIds[name]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the casing is the same of destination names in maybeBundledConfigIds and analytics.Integrations e.g. Google Analytics === Google Analytics vs google-analytics.. but perhaps something to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, the casing here is the same but they depend on two different sources.

- `bundled` depends on the name that is defined within the integration code itself: https://github.com/segmentio/analytics.js-integrations/blob/master/integrations/google-analytics/lib/index.js#L37
- `bundledConfigIDs` depends on the defintiion creation name: https://github.com/segmentio/ajs-renderer/pull/256/files#diff-ca733fb7a822baffec24f4f426ccb07fce89f5b4d5856f848be4845d1d40e12bR44

The chance of these being different is unlikely but possible. Fwiw, the integration wouldn't receive the proper settings if the name within the integration code doesn't match the destination creation name because settings are keyed by the definition creation name.

Copy link
Contributor

@briemcnally briemcnally Feb 9, 2021

Choose a reason for hiding this comment

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

+1 to Gabe's comment, I spot checked a few destinations which I know have undergone name changes and the name defined in integration code marched the definition creation name.

continue
}

for (var j = 0; j < maybeBundledConfigIds[name].length; j++) {
var id = maybeBundledConfigIds[name][j]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we maybe want to see if maybeBundledConfigIds[name] has more than one configId? In example case where maybeBundledConfigIds looks like the below, wouldn't we want to collect stats since that would mean they have multiple device mode configs of a destination in device mode, which is not allowed?

maybeBundledConfigIds: {
       Fullstory: [ config1, config2]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

bundledConfigIds.push(id)
}
}


msg._metadata = msg._metadata || {};
msg._metadata.bundled = bundled;
msg._metadata.unbundled = this.options.unbundledIntegrations;
msg._metadata.bundledConfigIds = this.options.bundledConfigIds;
msg._metadata.unbundledConfigIds = this.options.unbundledConfigIds;
msg._metadata.bundledIds = bundledConfigIds;
}
this.debug('normalized %o', msg);
this.ampId(ctx);
Expand Down
2 changes: 1 addition & 1 deletion integrations/segmentio/package.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "@segment/analytics.js-integration-segmentio",
"description": "The Segmentio analytics.js integration.",
"version": "4.3.1",
"version": "4.4.0",
"keywords": [
"analytics.js",
"analytics.js-integration",
Expand Down
32 changes: 30 additions & 2 deletions integrations/segmentio/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,9 @@ describe('Segment.io', function() {
segment = new Segment(options);
ajs.use(Segment);
ajs.use(integration('other'));
ajs.use(integration('another'));
ajs.add(segment);
ajs.initialize({ other: {} });
ajs.initialize({ other: {}, another: {} });
});

it('should add a list of bundled integrations when `addBundledMetadata` is set', function() {
Expand All @@ -459,7 +460,7 @@ describe('Segment.io', function() {

assert(object);
assert(object._metadata);
assert.deepEqual(object._metadata.bundled, ['Segment.io', 'other']);
assert.deepEqual(object._metadata.bundled, ['Segment.io', 'other', 'another']);
});

it('should add a list of unbundled integrations when `addBundledMetadata` and `unbundledIntegrations` are set', function() {
Expand All @@ -478,6 +479,33 @@ describe('Segment.io', function() {
assert(object);
assert(!object._metadata);
});

it('should generate and add a list of bundled destination config ids when `addBundledMetadata` is set', function() {
segment.options.addBundledMetadata = true;
segment.options.maybeBundledConfigIds = {
'other': ['config21'],
'slack': ['slack99'] // should be ignored
};
segment.normalize(object);

assert(object);
assert(object._metadata);
assert.deepEqual(object._metadata.bundledIds, ['config21']);
});

it('should generate a list of multiple bundled destination config ids when `addBundledMetadata` is set', function() {
segment.options.addBundledMetadata = true;
segment.options.maybeBundledConfigIds = {
'other': ['config21'],
'another': ['anotherConfig99'],
'slack': ['slack99'] // should be ignored
};
segment.normalize(object);

assert(object);
assert(object._metadata);
assert.deepEqual(object._metadata.bundledIds, ['config21', 'anotherConfig99']);
});
});

it('should pick up messageId from AJS', function() {
Expand Down