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

Added support for docExpansion config #3159

Merged
merged 4 commits into from
Jun 6, 2017

Conversation

gwynjudd
Copy link
Contributor

@gwynjudd gwynjudd commented Jun 1, 2017

Resolves #2799

@hlubovac
Copy link

hlubovac commented Jun 1, 2017

I tested this locally by adding docExpansion: "none" to swagger-config.yaml` file, and it worked: initial page-load displays endpoints collapsed, like this:
image

I tested setting docExpansion to NULL, empty or a bogus string literal, and it always fell back to original default behavior of showing endpoints uncollapsed, like this:
image

@webron webron requested a review from shockey June 1, 2017 15:58
@webron
Copy link
Contributor

webron commented Jun 1, 2017

@gwynjudd Thanks for taking the time to write this PR.

While @shockey reviews it, I was wondering whether you have the time to implement the full functionality of docExpansion as it were in 2.x. This means it would also support list (which is the default behavior), and full, which expands all the tags and operations. No worries if you don't have the time to tackle it.

Regardless, please add the docExpansion to the README.

@gwynjudd
Copy link
Contributor Author

gwynjudd commented Jun 1, 2017

@webron I updated it to support the other doc expansion settings 'full' and 'list', but I got stuck with an unexpected bug on 'full'. The first time it loads the page, it appears as this:

image

On debugging, it appears that the operations list loads and displays correctly, but at the very last, it is replaced with an empty list which for some reason renders as this. I got stuck trying to follow the logic of how the operations list is created and how this might be happening. If you have any suggestions, please let me know

@webron
Copy link
Contributor

webron commented Jun 1, 2017

Huh, interesting. @shockey?

@shockey
Copy link
Contributor

shockey commented Jun 5, 2017

Digging in!

@shockey
Copy link
Contributor

shockey commented Jun 6, 2017

So, this was a fun one to track down..

<ParamBody> was throwing an error TypeError: Cannot read property 'get' of undefined.

<ParamBody> gets its data from <Operations> which selects from the taggedOperations spec selector. taggedOperations uses operationsWithTags as an iterator, which is a composite selector of operationsWithRootInherited, which itself is a composite of operations, consumes, and produces.

Those three selectors are composites of the spec spec selector - which provides the resolved spec if it exists, or the raw JS spec if resolved is not yet available. This was the crux of the problem, because....

an unrelated call to the changeConsumesValue spec action is populating the resolved spec state with an empty tree, except for one parameter in one operation, so the resolved spec tree looks like this:

paths: {
  '/pet': {
    post: {
      parameters: [
        {}
      ],
      produces_value: 'application/xml'
    }
  }
}

This causes the next call to getParameter in <ParamBody> to fail, since it uses the spec selector that switches over once the resolved spec is defined. It fails because it's trying to query for a parameter that doesn't exist, because the resolved spec is incomplete.

That's also the reason that we were seeing one operation, the POST /pet operation, in the UI when it failed - the selectors were grabbing the resolved spec above and rendering it.


To fix this, I removed the switching logic from the spec spec selector, so that it always provides the resolved spec. This will prevent the operations from being rendered until the resolved spec is available, which will also delay the changeConsumesValue causing the problem.

This didn't appear to break anything, but I'll do some more extensive testing before merging and before releasing.

@shockey shockey merged commit 12dd90f into swagger-api:master Jun 6, 2017
@gwynjudd gwynjudd deleted the doc-expansion branch June 6, 2017 03:35
@webron
Copy link
Contributor

webron commented Jun 6, 2017

@gwynjudd thanks for all the work you've put into it, definitely helped us move forward. Hoping to see future contributions from you as well, time permitting of course :)

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.

4 participants