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

Conversation

aultimus
Copy link
Contributor

@aultimus aultimus commented May 20, 2020

This PR adds destination middleware support. This feature was originally released to master but was then reverted as a regression was discovered. That regression was addressed by this commit ( 010219b ) and we are re-releasing.

Verification

Testing completed successfully on local and staging. Run this code in the console of a project connected to a mixpanel destination and then observe the identify event in mixpanel with user id foobarcat

f=function({payload, next, integrations}) {payload.obj.userId = "foo"; next(payload);}; analytics.addSourceMiddleware(f);
f=function(payload, integration, next) {payload.obj.userId = payload.obj.userId + "bar";next(payload);}; analytics.addIntegrationMiddleware(f);
f=function({payload, integration, next}) { payload.obj.userId = payload.obj.userId + "cat";next(payload);}; analytics.addDestinationMiddleware("Mixpanel",[f]);
analytics.identify();

See:
https://www.dropbox.com/s/ncrbwm3tkvphhzf/Screen%20Recording%202020-05-21%20at%202.26.55%20PM.mov?dl=0

Checklist

Please ensure the following are completed to help get your PR merged:

  • Thorough explanation of the issue/solution, and a link to the related issue
  • CI tests are passing
  • Unit tests were written for any new code
  • Code coverage is at least maintained, or increased.

Respect earns Respect 👏

Please respect our Code of Conduct, in short:

  • Using welcoming and inclusive language.
  • Being respectful of differing viewpoints and experiences.
  • Gracefully accepting constructive criticism.
  • Focusing on what is best for the community.
  • Showing empathy towards other community members.

Matthew Ault added 9 commits May 20, 2020 13:25
This bug meant that this code did not send an event with a userId=bar
to segment.io. This change will result in this code sending an event
with userId=bar to segment.io:

f=function(payload, integration, next) {
	payload.obj.userId = "bar";next(payload);
};
analytics.addIntegrationMiddleware(f);
analytics.identify();
@aultimus aultimus requested a review from a team as a code owner May 20, 2020 20:26
@aultimus aultimus requested a review from pooyaj May 21, 2020 21:36
self._destinationMiddlewares[integration.name].applyMiddlewares(
facadeCopy,
integration.name,
function(result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way, using JSDocs, document the format we expect result to respect?

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 think you would need to do one line per property like this https://jsdoc.app/tags-param.html#parameters-with-properties

@juliofarah
Copy link
Contributor

juliofarah commented May 21, 2020

The code and PR documentation look good to me - will defer the final +1 to someone with more context!
As I don't have a lot of context on AJS as a whole, a newbie question I have is: although this PR is pretty well covered in terms of unit tests, how do we treat end-to-end and integration testing when implementing features like this?

Copy link
Contributor

@pooyaj pooyaj left a comment

Choose a reason for hiding this comment

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

Let's fix two comments, otherwise looks good to me.

HISTORY.md Outdated
@@ -1,3 +1,6 @@
- Revert "Add destination mw (#148)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the "Revert" line, since it will be i the release log, and confuses people

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved by fccfece

@@ -495,7 +495,7 @@ describe('Analytics', function() {
});

it('should emit "invoke" with facade', function(done) {
var opts = { All: false };
var opts = { All: true };
Copy link
Contributor

@pooyaj pooyaj May 22, 2020

Choose a reason for hiding this comment

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

can we revert this change? I wonder if we need this change even after fixing the emitter issue. Want to make sure all of the old tests are green without any modification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved by fccfece

@aultimus aultimus merged commit 497822c into master May 26, 2020
@aultimus aultimus deleted the add-dest-mw-take-two branch May 26, 2020 18:18
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.

3 participants