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

[FEATURE] Add callback support #1102

Conversation

Jonathan-Bailey-Bose
Copy link
Contributor

@Jonathan-Bailey-Bose Jonathan-Bailey-Bose commented Nov 13, 2019

Description

This PR utilizes the code from #757 written by @Makashov to add callback support to ReDoc. It further adds to the code by adding the following:

  • Adds isCallback: boolean field to the OperationModel class in order to distinguish a normal Operation from a Callback. This defaults to false.
  • Callbacks are defined with the following behavior as a type of Operation:
    • Callbacks by default will use the event name for the view's label when rendered by ReDoc, as opposed to the operationID.
    • Callbacks by default do not inherit the specification's global security definition(s). They default to having no security, and can have security added on an individual basis.
    • Callbacks by default do inherit the specification's global servers definition(s). In many cases, this can be undesirable. Override the servers property on individual callbacks to remedy this if needed.
  • Adds some tests to maintain code coverage.
  • The rendering style of a callback is meant to mimic that of an operation within an operation response dropdown.

@sundbry
Copy link

sundbry commented Nov 21, 2019

I gave this a shot and it worked flawlessly with our API spec. Thank you! It did add some extra debug logging to the output which I imagine should be removed before merging.

@Jonathan-Bailey-Bose
Copy link
Contributor Author

@sundbry Glad to hear it! Thanks for the catch, forgot to remove that pesky line 😉

@RomanHotsiy
Copy link
Member

Hey @Jonathan-Bailey-Bose! Thanks for your PR, we're super busy right now.

I hope I will be able to review and merge this PR early Feb

@bjokax
Copy link

bjokax commented Feb 14, 2020

Do we know if this can be merged soon!? We are using many resthooks/webhooks in our product and it would be nice to be able to document them properly with redoc!

@asyncee
Copy link

asyncee commented Feb 15, 2020

It would be great to have this feature in production.

@jchook
Copy link

jchook commented Feb 16, 2020

Excited to see this merged as well. Looks great so far.

I did find a couple small bugs...

1. Servers

  • components.callbacks.Test./test.post.servers does show in the UI
  • components.callbacks.Test./test.servers does not show in the UI

2. Scrolling menu

Something about adding the callback section makes the menu disappear/jump to the top during scrolling. It appears to happen as you scroll into (or out of) a tag section that contains callbacks.

It also renders the menu inert.

For me, it only happens on Chromium 80 and not on FireFox 73.

Peek 2020-02-16 13-57 callbacks mess up menu follow

@bjokax
Copy link

bjokax commented Feb 17, 2020

I think the Chrome 80 issue has been already fixed in the latest RC release!

@Zhendryk
Copy link

Zhendryk commented Feb 17, 2020

See #1167, I don't believe this branch has been rebased on top of that fix yet, but the fix exists in v2.0.0-rc.23

@Jonathan-Bailey-Bose
Copy link
Contributor Author

Branch has been rebased ontop of latest changes, including the Chrome 80 fix.

@Jonathan-Bailey-Bose
Copy link
Contributor Author

@RomanHotsiy Any idea of when this will be reviewed and merged?

@VersBinarii
Copy link

It would be really great to have this PR merged

@girishadat
Copy link

I hope the reviews for this PR is in progress. It would be great to have an RC build with this feature! @RomanHotsiy @Jonathan-Bailey-Bose

@RomanHotsiy
Copy link
Member

Hey @Jonathan-Bailey-Bose!

Sorry for the huge delay. We're super busy. Thanks for your contribution again!

I found some time today to review the functionality of this PR 🎉

I have a few points:

  1. There is a major issue with enpoint (see screenshot):
    image

You use <Endpoint> component which is not adapted to callbacks. I believe that having URL in the middle panel would be more useful for callbacks, so I would put it next to the callback name, see screenshot for example:

image

  1. Responses for callback is easy to confuse with general responses. Maybe we should wrap it into some background or maybe you have some better idea.

Are you willing to apply the changes? I can't ask you as you already put so much efforts into this and hit the huge delays from our side 😞. I can pick it up too.

Thanks

@RomanHotsiy
Copy link
Member

Superseded by #1224

@RomanHotsiy RomanHotsiy closed this Apr 7, 2020
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

9 participants