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(schema) support hook to poll triggers INT-15776 #432

Merged
merged 9 commits into from
Nov 11, 2021
Merged

Conversation

casshill
Copy link
Contributor

@casshill casshill commented Oct 11, 2021

Add support for hook_to_poll style triggers (new schema)

  • soft launch by hiding the doc references ie. this pr should not generate changes to the docs
  • require performList and exclude perform.
    • Perform is a no-op for hook-to-poll triggers, the live Zap run calls performList (since it's essentially polling)
    • Should performSubscribe and performUnsubscribe be required? I think they are only optional for legacy hook support

@casshill casshill self-assigned this Oct 11, 2021
@casshill casshill changed the title feat(schema) support hook to poll triggers feat(schema) support hook to poll triggers INT-15776 Oct 12, 2021
@casshill casshill requested a review from xavdid October 13, 2021 05:08
Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

@casshill is this ready for review? It's still marked as draft, so I wasn't sure.

In any case, looks good! I'm not sure how much you care about hiding the new value from the docs, but we have some code for that in here:

if (_.get(property, 'docAnnotation.hide')) {
return '';
} else if (_.get(property, 'docAnnotation.required')) {
// can also support keys besides "required"
const annotation = property.docAnnotation.required;
if (annotation.type === 'replace') {
isRequired = annotation.value;
} else if (annotation.type === 'append') {
isRequired += annotation.value;
} else {
throw new Error(`unrecognized docAnnotation type: ${annotation.type}`);
}
}
return `${quoteOrNa(key)} | ${isRequired} | ${typeOrLink(property)} | ${
property.description || NO_DESCRIPTION
}`;

We don't have anything for hiding a specific enum, but we could add it.

@casshill
Copy link
Contributor Author

@casshill is this ready for review? It's still marked as draft, so I wasn't sure.

In any case, looks good! I'm not sure how much you care about hiding the new value from the docs, but we have some code for that in here:

if (_.get(property, 'docAnnotation.hide')) {
return '';
} else if (_.get(property, 'docAnnotation.required')) {
// can also support keys besides "required"
const annotation = property.docAnnotation.required;
if (annotation.type === 'replace') {
isRequired = annotation.value;
} else if (annotation.type === 'append') {
isRequired += annotation.value;
} else {
throw new Error(`unrecognized docAnnotation type: ${annotation.type}`);
}
}
return `${quoteOrNa(key)} | ${isRequired} | ${typeOrLink(property)} | ${
property.description || NO_DESCRIPTION
}`;

We don't have anything for hiding a specific enum, but we could add it.

It's ready for feedback and suggestions :D
I thought I'd leave it as draft until we decided how/when we wanted to release it - thanks I'll take a look at the snippet you provided

@casshill
Copy link
Contributor Author

casshill commented Oct 14, 2021

@xavdid I couldn't see how to make the doc annotations hide for sub-components of a property, it seemed to be more for full property blocks?

Eg I don't think I can show docs for one enum value 'hook', but not for the second enum value 'hook_to_poll' (only for the whole enum property)

It might make more sense (added in the latest commit) to make a new operation schema, particular as we need to use performList instead of perform. What do you think of that as opposed to adding a new enum to the basic hook operation schema? I think we have the same issue with hiding the docs though. It's probably more subtle to just add an enum value if we don't want to publish the full docs though, so perhaps then enum is better for the soft launch and the new operation schema for the hard launch

@xavdid
Copy link
Contributor

xavdid commented Oct 14, 2021

Right, there's not currently support for hiding a specific property. so we could add support for that, or do something different (such as a new schema).

If hook-to-poll doesn't have a perform at all, then I think it does make sense to have it as a new schema.

Copy link
Contributor

@xavdid xavdid left a comment

Choose a reason for hiding this comment

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

Nice! this is looking good.

I haven't taken a super close look, but if you've done some testing then this should be safe enough! We'll want to push a test app to staging and actually make sure it all runs, but it should be pretty straightforward.

packages/schema/exported-schema.json Outdated Show resolved Hide resolved
@casshill casshill marked this pull request as ready for review November 10, 2021 01:43
@casshill casshill requested a review from xavdid November 10, 2021 23:56
@casshill casshill merged commit c02be64 into master Nov 11, 2021
@casshill casshill deleted the hook-to-poll branch November 11, 2021 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants