Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

z.cursor store #76

Merged
merged 7 commits into from
May 31, 2018
Merged

z.cursor store #76

merged 7 commits into from
May 31, 2018

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Apr 4, 2018

Covers PDE-142.

This is being tested with this trigger

Note: https://github.com/zapier/zapier/pull/16646 needs to be deployed before this can be released

See linked z/z PR for testing info.

CI is failing because it's still pointing at my local ngrok for testing reasons. I'll remove that before this is merged.

@xavdid xavdid requested a review from BrunoBernardino April 19, 2018 05:07
@xavdid xavdid changed the title [WIP] z.cursor store z.cursor store Apr 19, 2018
Copy link
Contributor

@BrunoBernardino BrunoBernardino left a comment

Choose a reason for hiding this comment

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

Great stuff! Love the simplicity of this API and the tests.

I didn't pull it down as it's a bit convoluted to test, and you've got test coverage.

I've got a few questions/#suggestions, but not really blocking.

// have a storeKey when canPaginate is true. otherwise, a test would work but a
// poll on site would fail. this is only used in test handlers
const shouldPaginate = (appRaw, method) => {
const methodParts = method.split('.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this more easily understood with method.startsWith('triggers') && method.endsWith('perform'), then not needing last from lodash?

Feel free to ignore as you need the methodParts below anyway.

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 need to split it anyway, but I don't like checking on the string rather than the array. good call!

const _ = require('lodash');
const ZapierPromise = require('./promise');

// Similar API to JSON built in but catches errors with nicer tracebacks.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this comment came from copy-🍝 .

@@ -29,4 +29,26 @@ describe('rpc client', () => {
})
.catch(done);
});

it('should set a cursor key', done => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love to see a test getting a cursor that doesn't exist, and setting an invalid store key (like undefined or null or a gigantic string), just so we can be sure failures are failing 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah! getting a cursor that hasn't been set is fine and returns null (just like redis does). A store key missing is an error, and that behavior has a unit test on the server. I can go ahead and add something there though!

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'm having trouble testing this because since we're not actually sending any calls, nothing actually fails unless I explicitly fail it, which is then just the same. Given that there's actually a server test, i'll skip here for now.

@xavdid xavdid merged commit be7a25f into master May 31, 2018
@xavdid xavdid deleted the cursor-store branch May 31, 2018 06:51
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.

2 participants