Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(splitio-api-binding): add function to poll for split changes #5

Merged
merged 8 commits into from
Dec 13, 2019
9 changes: 0 additions & 9 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,3 @@ matrix:
include:
- node_js: "12"
- node_js: "10"
- node_js: "8"
# https://github.com/greenkeeperio/greenkeeper-lockfile#npm
before_install:
# package-lock.json was introduced in npm@5
- '[[ $(node -v) =~ ^v9.*$ ]] || npm install -g npm@latest' # skipped when using node 9
- npm install -g greenkeeper-lockfile
before_script: greenkeeper-lockfile-update
after_script: greenkeeper-lockfile-upload
install: npm install
34 changes: 32 additions & 2 deletions lib/splitio-api-binding.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const https = require('https')
const axios = require('axios')

const SPLITIO_API_URI = 'https://sdk.split.io/api'
const SINCE_VALUE_FOR_FIRST_REQUEST = -1

const httpsAgent = new https.Agent({
keepAlive: true,
Expand Down Expand Up @@ -53,11 +54,40 @@ class SplitioApiBinding {
}
}

/**
* Polls the Split.io API until since and till timestamps are the same.
*/
async getAllChanges ({ path }) {
let since = SINCE_VALUE_FOR_FIRST_REQUEST
let results = await this.httpGet({ path, since })
const allChanges = [results]
// The till value represents the timestamp of the last change included in the response.
let till = results.till
while (since !== till) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these values guaranteed to converge? I am worried that there would be a case where this runs infinitely.

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 they will -- it would only run a long time if there's a huge number splits

from the doc from split:

Ideally you would keep fetching changes until this condition is fulfilled, instead of just assuming one request will get you everything. 
After this condition is true, you periodically poll for updates.
There will only be updates to the data if this condition is NOT true on a given response.

Copy link
Contributor

Choose a reason for hiding this comment

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

The potentially infinite loop makes me nervous. E.g., if split.io API has a bug, that bubbles up as a infinite loop in our product code. Can we add a limit on the loop?

Copy link
Contributor

Choose a reason for hiding this comment

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

does this work?

const since = SINCE_VALUE_FOR_FIRST_REQUEST
const allChanges = []
while (true) {
  const results = await this.httpGet({ path, since })
  if (since === results.till) {
    break
  }
  allChanges.push(results)
  since = results.till
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, so much nice

since = till
results = await this.httpGet({ path, since })
till = results.till
if (since === till) {
return { allChanges, since }
}
allChanges.push(results)
}
return { allChanges, since }
}

/**
* Get split data.
*/
getSplitChanges () {
throw new Error('Not implemented')
async getSplitChanges () {
const path = '/splitChanges'
const splitChanges = []
const { allChanges, since } = await this.getAllChanges({ path })
allChanges.forEach(change => {
change.splits.forEach(split => {
splitChanges.push(split)
})
})
return { splitChanges, since }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why this function returns since, will we use it somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, the FE data loader will use it so that the SDK knows what timestamp to use for polling for updates

Copy link
Contributor Author

Choose a reason for hiding this comment

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

edit: talked to silas more about this -- we'll eventually keep track of this on the backend but are just going to poll with -1 for now

}

/**
Expand Down
72 changes: 67 additions & 5 deletions lib/splitio-api-binding.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const { expect } = require('chai')
const nock = require('nock')
const sinon = require('sinon')

const SplitioApiBinding = require('./splitio-api-binding')

Expand Down Expand Up @@ -74,15 +75,76 @@ describe('lib.splitio-api-binding.SplitioApiBinding', () => {
})
})

describe('getAllChanges', () => {
const mockChanges0 = [{ bar: 'bar' }]
const mockChanges1 = [{ baz: 'baz' }]
it('returns all changes and the since value', async () => {
const splitioApiBinding = new SplitioApiBinding({ splitioApiKey: 'foo' })
const requestStub = sinon.stub()
splitioApiBinding.httpGet = requestStub
requestStub.onCall(0).resolves({ changes: mockChanges0, till: 0 })
requestStub.onCall(1).resolves({ changes: mockChanges1, till: 1 })
requestStub.onCall(2).resolves({ changes: mockChanges1, till: 1 })
const changes = await splitioApiBinding.getAllChanges({ path: '/mock' })
expect(changes).deep.equals({
allChanges: [
{ changes: mockChanges0, till: 0 },
{ changes: mockChanges1, till: 1 }
],
since: 1
})
})

it('throws an error on an error from httpGet', async () => {
const splitioApiBinding = new SplitioApiBinding({ splitioApiKey: 'foo' })
const requestStub = sinon.stub()
splitioApiBinding.httpGet = requestStub
requestStub.onCall(0).resolves({ changes: mockChanges0, till: 0 })
requestStub.onCall(1).rejects('Some Error')
try {
await splitioApiBinding.getAllChanges({ path: '/mock' })
throw new Error('did not throw')
} catch (err) {
expect(err).to.be.an.instanceof(Error)
}
})
})

describe('getSplitChanges', () => {
it('should throw a not implemented error', () => {
const mockSplits0 = { status: 'bar' }
const mockSplits1 = { status: 'baz' }
it('returns all splits and the since value', async () => {
const splitioApiBinding = new SplitioApiBinding({ splitioApiKey: 'foo' })
const requestStub = sinon.stub()
splitioApiBinding.getAllChanges = requestStub
requestStub.onCall(0).resolves({
allChanges: [
{ splits: [mockSplits0], till: 0 },
{ splits: [mockSplits1], till: 1 }
],
since: 1
})
const splitChanges = await splitioApiBinding.getSplitChanges({ path: '/splitChanges' })
expect(splitChanges).deep.equals({
splitChanges: [
mockSplits0,
mockSplits1
],
since: 1
})
})

it('throws an error on an error from getAllChanges', async () => {
const splitioApiBinding = new SplitioApiBinding({ splitioApiKey: 'foo' })
const requestStub = sinon.stub()
splitioApiBinding.getAllChanges = requestStub
requestStub.onCall(0).rejects('Some Error')
try {
splitioApiBinding.getSplitChanges()
} catch (e) {
return expect(e.message).equals('Not implemented')
await splitioApiBinding.getSplitChanges()
throw new Error('did not throw')
} catch (err) {
expect(err).to.be.an.instanceof(Error)
}
throw new Error('Test did\'t throw')
})
})

Expand Down
110 changes: 110 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,13 @@
"mocha": "6.2.2",
"nock": "11.7.0",
"nyc": "14.1.1",
"sinon": "^7.5.0",
"standard": "14.3.1",
"standard-version": "7.0.1"
},
"nyc": {
"check-coverage": true,
"lines": 100,
"lines": 97,
"functions": 100
}
}