Integration.prototype.map — add support for array type #45
Conversation
|
ping :) |
| if (type(events) === 'array') { | ||
| // If there's no key attached to this event mapping (unusual), skip this | ||
| // item. | ||
| if (!val.key) return matchingEvents; |
There was a problem hiding this comment.
This case no longer gets handled, which means if key is a non-string (easy way for this to happen is null or just missing) then we try to do string replacement on a non-string and get an uncaught exception.
Example case:
it('should handle non-string keys gracefully', function() {
var obj = [{ key: null, value: '1121f10f' }];
assert([], integration.map(obj, 'event'));
});
ndhoule
changed the title
|
This isn't a regression, this method was never designed to support arrays of strings |
| // Extract the key and value from the `mixed` object. | ||
| key = val.key; | ||
| val = val.value; | ||
| switch (type(val)) { |
There was a problem hiding this comment.
switches are rarely clearer than ifs and are more error prone, prefer if
There was a problem hiding this comment.
Per usual, just following precedent: https://github.com/segmentio/analytics.js-integration/blob/master/lib/protos.js#L244-L266
I'm all for clarity, but if we are going to start imposing rules going forward we need a style guide so that we have an opportunity to catch ourselves before asking for review.
| @@ -367,9 +367,9 @@ describe('integration', function(){ | |||
| }); | |||
|
|
|||
| describe('when .options.events is an array', function(){ | |||
There was a problem hiding this comment.
Restructure these tests more clearly around the different types we intend for this to handle, which will give clarity to exactly what map can handle and what it can't
There was a problem hiding this comment.
definitely. good call.
sperand-io
force-pushed
the
map/array
branch
2 times, most recently
from
61765ec to
1f781c1
Compare
|
Updated in line with comments (thanks!). You're right, looks like this never supported arrays in the first place, but it definitely should. The steelhouse integration is the first that really requires this functionality, and the faster we get that fix out the better. |
| if (type(mapping) === 'array' && every(isString, mapping)) return 'array'; | ||
| if (type(mapping) === 'array' && every(isMixed, mapping)) return 'mixed'; | ||
| return 'unknown'; | ||
| } |
There was a problem hiding this comment.
These functions get unnecessarily redeclared every time we call #map; move them outside this function scope
sperand-io
force-pushed
the
map/array
branch
2 times, most recently
from
2e149b9 to
838b9c4
Compare
|
think I got everything in latest pass. thanks for the careful eye and patient feedback |
| * | ||
| * @api private | ||
| * @param {Object} item | ||
| * @return {undefined} |
There was a problem hiding this comment.
Missing jsdoc return value
|
|
||
| it('should return single matched values', function(){ | ||
| var option = [{ key: 'bar', value: '4cff6219' }, { key: 'baz', value: '4426d54'} ]; | ||
| assert(['4426d54'], integration.map(option, 'baz')); |
There was a problem hiding this comment.
should be assert.deepEqual
|
We should probably migrate Kenshoo sometime to use this new functionality, since it's been using an array of events for a while now, and this solution is improved in that it normalizes event names so they're not quite so strict. (We could have just used that solution in Steelhouse in the interim, but oh well) I'm wrapping up the enterprise npm migration which is blocking deploys, so I'll let you fix this stuff while I wrap that up |
|
@ndhoule updated! |
Integration.prototype.map — add support for array type
The refactors to prototype#map in [#45][] are a little too presumptuous in that they assume the integration optons values stored in metadata are always going to be strings, which is not the case In the long run, trying to determine if something is an array or a map or a mixed on the client is a pretty bad solution. We're essentially reduced to duck typing metadata structures, which is not a good solution. Instead metadata should look like: ```js { "type": "mixed", "data": [ { key: "whatever", value: "whatever_value" }, // ... ] } ``` [#45]: #45
This PR adds support for mapping basic arrays of strings, a structure used in metadata to store conversion events for such integrations as Steelhouse:
@ndhoule