feat(schema) TN-169 Adds canPaginate to BasicHookOperationSchema #399
Conversation
This is required for hook triggers to paginate with cursors in performList - either for editor dropdowns, or Transfer support
| @@ -2666,7 +2666,7 @@ describe('triggers', () => { | |||
|
|
|||
| ### Using the `z` Object in Tests | |||
|
|
|||
| Introduced in `[email protected]`, `appTester` can now run arbitrary functions that are not in your app definition: | |||
There was a problem hiding this comment.
You may need to re-merge master into this PR, this line shouldn't be being removed.
There was a problem hiding this comment.
The line's being shortened, not removed. Not sure why, but it looks like the short version of the line was added to README-source.md in https://github.com/zapier/zapier-platform/pull/385/files#diff-3dde54695dbc62f7d2d2212cc889a7aeb3fbffee64355037ceaaba6755b616af and README.md is out of sync, maybe from some other branch? Building my changes included that one.
There was a problem hiding this comment.
huh! I guess something didn't get run when I added it originally. ¯\_(ツ)_/¯ sorry about that!
| @@ -51,6 +51,10 @@ BasicHookOperationSchema.properties = { | |||
| }, | |||
| }, | |||
| }, | |||
| canPaginate: { | |||
| description: 'Does this endpoint support a page offset?', | |||
There was a problem hiding this comment.
I realize this is the existing text, but this is a good opportunity to improve it. canPaginate also controls whether the cursorStore works. So maybe something like:
Does this trigger support pagination? Either via a numeric "page" offset or via temporary cursor storage.
that's a little wordy, but you get the idea.
There was a problem hiding this comment.
Hmm...now that I think about it, is this description actually correct? I think canPaginate is only required for the cursor store, and https://github.com/zapier/zapier/pull/16646 makes it sound like there could be a performance hit if triggers set canPaginate but don't use the cursor store. Maybe we should actually flip it?
Does this endpoint support pagination via temporary cursor storage?
There was a problem hiding this comment.
I know I've seen CLI triggers that use page offset without setting canPaginate, like https://github.com/zapier/zapier-platform-cli-apps/blob/f7c6523b357a792d052f021199ff96cc3612cfeb/apps/mixer/triggers/dynamic_dropdowns/list_follows.js
There was a problem hiding this comment.
yep, you're correct! I wonder why we even added this flag then - I think it predates the cursor store 🤔
There was a problem hiding this comment.
ha, In https://github.com/zapier/zapier/pull/16646, I said (in 2018):
I notice the BasicPollingOperationSchema mentions canPaginate, but that string doesn't show up anywhere in our codebase
So maybe this is all just a happy accident.
|
@xavdid Thanks! I merged, and then ran Do I need to push this commit to Edit: Hmm, I do see there is a Travis.io Edit 2: Doesn't look like it ran. I'll post in #team-dev-platform and await further instructions! |
This is required for hook triggers to paginate with cursors in performList - either for editor dropdowns, or Transfer support