Update everything! by jescalan · Pull Request #13 · customerio/customerio-node

jescalan · 2018-02-22T00:09:58Z

Hey friends,

So I rewrote this library almost completely. I know this is exactly the type of PR that I don't particularly enjoy seeing myself, but I felt like it was justified for a couple reasons:

The dependencies hadn't been touched in 4+ years
The API surface area was incomplete, and missing several officially supported methods
Javascript has changed and got a lot better in the last couple of years

Here's what I did:

Modernized all the javascript to es6. Prototype classes turned into real classes, external dependency promises to native promises, _this = this eliminated through arrow functions, all code formatted using prettier, etc.
Brought all dependencies up to date, made sure that no test dependencies were in the normal dependencies section (there was one big one previously).
Rewrote the tests using ava instead of mocha. Ava has a very strong integration with the nyc package for test coverage, is a little smaller and more clear, and doesn't pipe in a bunch of globals that you need to guess. The tests themselves are effectively the same despite this.
Added support for anonymous event tracking and the triggerBroadcast endpoint, and added test coverage for these as well
Eliminated a lot of repeated or useless code and generally decreased the footprint by a lot.

All tests are passing, test coverage has been improved, and I'm hoping you guys will accept this. I would be happy to maintain it to the best of my ability as things change - I know that I will be relying on this code for my job regardless, so it's a good fit.

Thank you for making an awesome email platform, and sending over much love from HashiCorp 💖

Also, this closes #12, and incorporates the suggestion

alvincrespo

This looks pretty amazing! I don't work at Customer.io anymore, but as the original author of this library - this should totally be approved.

Thanks for making this happen - you rock!

alvincrespo · 2018-02-22T00:26:24Z

@@ -1,13 +1,13 @@
-[![Build Status](https://travis-ci.org/alvincrespo/ember-cli-customerio.svg)](https://travis-ci.org/alvincrespo/ember-cli-customerio)
+[![Build Status](https://travis-ci.org/customerio/customerio-node.svg)](https://travis-ci.org/customerio/customerio-node)


alvincrespo · 2018-02-22T00:26:37Z


 ```
-npm install --save customerio-node
+npm i customerio-node


Should these be updated to yarn instead of npm?

I don't think so - npm is still the default so probably best to leave it. Although of course yarn will work as well!

alvincrespo · 2018-02-22T00:28:04Z

+    return typeof id === 'object'
+      ? this.request.post(`${root}/events`, id)
+      : this.request.post(`${root}/customers/${id}/events`, data)
+  }


Do you think it's worth making to methods: trackAsObject and trackAsId. I always worry about checking against types.

Yeah we could definitely separate them into different methods. I'd probably go with track and trackAnonymous or something like this. This API is a little bit smoother though, and I can't really think of an instance where someone would pass an object as the id and not expect something to go horribly wrong.

I'm open to either though tbh

Anonymous event tracking isn't exposed well by our API libraries. The Ruby library is the only one to have a method for anonymous event tracking today.

I'm in favor of being explicit in separating these API calls into their own methods, track and trackAnonymous. This is more inline with our approach with the Ruby lib and our API documentation which separates anonymous events into their own resource.

alvincrespo · 2018-02-22T00:28:41Z

-  "directories": {
-    "test": "tests"
+  "version": "0.2.0",
+  "author": "Alvin Crespo",


This should probably be CustomerIO?

🤷‍♂️I can change it to this if you want!

alvincrespo · 2018-02-22T00:29:00Z

-    "type": "git",
-    "url": "https://github.com/customerio/customerio-node.git"
+  "devDependencies": {
+    "ava": "^0.25.0",


jescalan · 2018-02-22T00:37:12Z

@alvincrespo 👬 💖

jescalan · 2018-02-22T00:39:50Z

Hey also, I feel like we should probably ask @liamdon if he would mind giving us the primary customer.io namespace, especially if this one is hosted/maintained by the company itself.

joepurdy

Thank you so much @jescalan for giving our node library some much needed TLC! 😍

I've left a couple comments for you to review, but we're also going to have one of our front end engineers review this PR since they're more well versed in JS. 😀

joepurdy · 2018-03-13T18:27:23Z

+    return typeof id === 'object'
+      ? this.request.post(`${root}/events`, id)
+      : this.request.post(`${root}/customers/${id}/events`, data)
+  }


Anonymous event tracking isn't exposed well by our API libraries. The Ruby library is the only one to have a method for anonymous event tracking today.

I'm in favor of being explicit in separating these API calls into their own methods, track and trackAnonymous. This is more inline with our approach with the Ruby lib and our API documentation which separates anonymous events into their own resource.

joepurdy · 2018-03-13T18:36:30Z

+  }
+
+  triggerBroadcast(id, data, recipients) {
+    return this.request.post(`${root}/campaigns/${id}/triggers`, {


This won't actually work for triggered broadcasts because they use a different API endpoint. https://api.customer.io/v1/api/campaigns/${id}/triggers instead of https://track.customer.io/api/v1/campaigns/${id}/triggers

For now only track.customer.io API methods are in the libraries because we're still determining the best path forward to supporting the two distinct APIs we provide.

Good catch! I'll update this now, and will also separate the track method in the way you described. I considered this initially, but decided to go the way I did because it's the same endpoint, and it was a little smoother of an interface. But the rationale you have there makes perfect sense and I'll change it over.

liamdon · 2018-03-13T18:49:10Z

Hey guys! Great to see all this activity on the library. I would definitely like to give you the npm namespace. What's the best way to do this? Could I add someone as an owner on npm to make the required changes?

joepurdy · 2018-03-13T19:07:34Z

 const Request = require('./request')
-const root = 'https://track.customer.io/api/v1'
+const trackRoot = 'https://track.customer.io/api/v1'
+const apiRoot = 'https://api.customer.io/v1'


So close!

You just need to add /api to change the apiRoot to 'https://api.customer.io/v1/api' here and in test/index.js. Beyond that the changes look spectacular. 🤩

Oh wow, don't know how I missed that one. Fix incoming!

joepurdy · 2018-03-13T20:12:54Z

 const Request = require('./request')
-const root = 'https://track.customer.io/api/v1'
+const trackRoot = 'https://track.customer.io/api/v1'
+const apiRoot = 'https://api.customer.io/api/v1'


Still not quite right, it's a bit confusing because the order of api/v1/ gets flipped on the apiRoot endpoint. So instead of 'https://api.customer.io/api/v1' it should be 'https://api.customer.io/v1/api'.

In your defense when I originally added triggered broadcasts to our API documentation I also mixed up the order about five or six times before getting it right. 😂

hahah good lord, who was responsible for this decision?! oook one more update coming soon

ook let's see if this round did it!

kevinkucharczyk

Oh wow, thank you soo much for this amazing PR! Looks great to me 🙂

joepurdy · 2018-03-16T19:31:34Z

@jescalan We're going to merge this PR and bump the minor version on Monday. 🍾

@liamdon Thanks for the offer to transition the customer.io namespace to us, however we discussed it internally and decided to keep the package names as they are today to avoid introducing pain for any current users of customer.io package that aren't locking their version safely.

jescalan · 2018-03-16T19:37:40Z

🙌 🎉

jescalan · 2018-03-19T16:34:05Z

Excited to see this land! Could you do me a favor and let me know when it has been released to NPM, just so I can update our code from using the fork to the official? 🥇

joepurdy · 2018-03-19T17:32:39Z

fyi @jescalan v0.3.0 has landed!

🙌🎉😤

Jeff Escalante added 8 commits February 21, 2018 14:53

modernize all code and dependencies, rewrite tests using ava and nyc

013c96d

replace rsvp with native promise, improve promise assertion test

7208770

adjust file naming and package.json

18b2dc3

a couple fixes to the readme, remove empty changelog

c834cfb

adjust travis params, implement pr customerio#12 upstream

b0e8dbd

add support and tests for triggerBroadcast

b27ba73

add docs for additional methods

c0e6504

remove yarn lock file

8fb24be

alvincrespo reviewed Feb 22, 2018

View reviewed changes

joepurdy suggested changes Mar 13, 2018

View reviewed changes

joepurdy requested a review from kevinkucharczyk March 13, 2018 18:58

joepurdy reviewed Mar 13, 2018

View reviewed changes

jescalan force-pushed the master branch from 72f7426 to 9f46f79 Compare March 13, 2018 19:57

joepurdy reviewed Mar 13, 2018

View reviewed changes

add trackAnonymous method, correct api base for triggerBroadcast

0d2e932

jescalan force-pushed the master branch from 9f46f79 to 0d2e932 Compare March 13, 2018 20:25

joepurdy approved these changes Mar 13, 2018

View reviewed changes

kevinkucharczyk approved these changes Mar 16, 2018

View reviewed changes

joepurdy merged commit acfe410 into customerio:master Mar 19, 2018

		@@ -1,13 +1,13 @@
		[![Build Status](https://travis-ci.org/alvincrespo/ember-cli-customerio.svg)](https://travis-ci.org/alvincrespo/ember-cli-customerio)
		[![Build Status](https://travis-ci.org/customerio/customerio-node.svg)](https://travis-ci.org/customerio/customerio-node)

Conversation

jescalan commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alvincrespo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jescalan commented Feb 22, 2018

Uh oh!

jescalan commented Feb 22, 2018

Uh oh!

joepurdy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamdon commented Mar 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinkucharczyk left a comment

Choose a reason for hiding this comment

Uh oh!

joepurdy commented Mar 16, 2018

Uh oh!

jescalan commented Mar 16, 2018

Uh oh!

jescalan commented Mar 19, 2018

Uh oh!

joepurdy commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jescalan commented Feb 22, 2018 •

edited

Loading