Lazy resolution - Githubissues

shockey commented 6 years ago

Description

Lazy resolution will allow us to handle larger API definitions more effectively in Swagger-UI.

We currently run entire API definitions through the Swagger-Client resolver when the definition is loaded into the UI. We can save a lot of effort by having the UI request resolved data as needed.

Conveniently, $ref usage is restricted in the Swagger and OpenAPI Specifications, so we can rely on having enough data to populate a list of operations and models without resolving anything, and only resolve things when users interact with the UI.

Plan

Create specActions.requestResolution([...path])
Create specSelectors.getResolvedSubtree([...path])
New flow:
- requestResolved ->
- is resolved data cached in state?
- yes? noop
- no? async SwC.resolveIn ->
  - update resolved state ->
  - re-render with updated state through getResolvedSubtree
Rework Models & Operations to request and consume resolved subtrees as needed
Support Swagger-Editor lifecycle in modified rendering (invalidate and re-request resolved subtrees when the spec content updates)
Move spec metadata out of resolved key, into a new meta key
Stop resolving by default (will need to provide an example to folks who will continue relying on specResolved)
Collapse models by default, so they don't need to be resolved until the user interacts with them

heldersepu commented 6 years ago

On the "Collapse model(s) by default" there are options to expand to a specific level

After this change we still can use that right?

shockey commented 6 years ago

@heldersepu, yes! the default has changed, each model in the models section will collapse by default (level 1 now means “display the container and name, but no content”); this is to avoid needing to do the work until the user expresses their intention to see the model.

how does that feel to you?

heldersepu commented 6 years ago

Sounds good. But this default value still configurable, right? if not then the configuration page should be updated

heldersepu commented 6 years ago

I think this will address some of the slow loading I'm getting here: http://petstore.swagger.io/?url=http://nhc-noaa.azurewebsites.net/swagger_ErliSoares.json&filter=_&defaultModelsExpandDepth=5

shockey commented 6 years ago

@heldersepu hopefully it will, however modifying the expand depth is going to negate a lot of the benefits 😉

heldersepu commented 6 years ago

ohhh I was thinking that the lazy load meant a visual delay... ...like some sort of animation expanding the models in a lazy way instead of waiting for the full rendered version

shockey commented 6 years ago

@heldersepu no worries! it's an ambiguous term 😄

here's a static build of the changes, you can try them out: http://devilish-coat.surge.sh/#/

heldersepu commented 6 years ago

Testing on your build: http://devilish-coat.surge.sh/?url=http://nhc-noaa.azurewebsites.net/swagger_ErliSoares.json&filter=_&defaultModelsExpandDepth=0 that loads a faster than on the petstore But if I increase the number to 5 it just crashes...

shockey commented 6 years ago

Yeah, I'm seeing that too. I'm playing with it locally, looks like there's a lot of redundant resolving going on with this definition - working on it now. Thanks for the heads up!

heldersepu commented 6 years ago

I was thinking that the expanding of the models could be on a timer and inside the timer we do something like: document.getElementsByClassName("model-toggle collapsed")[0].click()

shockey commented 6 years ago

@heldersepu, I like the idea in theory, but the resolver would still be blocking the main thread, so the UI would lock up for a spell without the user initiating work through a click, which feels not-great.

The idea behind that, though (resolving sequentially) may be part of the solution for the spec you provided.

I've isolated the issue to two definitions: Reserva and Patrimonio. They're either massive, or preserving some type of circular structure, which is causing the thread to crash. I'm not sure if it's a size problem or a loop problem, since I'm getting an out-of-memory error, not a call stack size error.

Stay tuned!

heldersepu commented 6 years ago

I guess I have to read on resolvers and how they block the main thread...

But here is a more concrete example on my idea: Let's start with the page: http://devilish-coat.surge.sh/?url=http://nhc-noaa.azurewebsites.net/swagger_ErliSoares.json&filter=_&defaultModelsExpandDepth=1 That renders fast Then open the browser Dev Tools and in the console paste the following:

function ExpandModel() {
    const o = document.getElementsByClassName("model-toggle collapsed")
    if (o.length > 0) {
        o[0].click()
        setTimeout(ExpandModel, 100);
    }
}

Then all we need to do is execute it ExpandModel() that will "slowly" and recursively expand all models without any visible blocking spells

shockey commented 6 years ago

@heldersepu gotcha! i'm partial to saying that triggering UI events programmatically would be creating more work then necessary for the UI, but feel free to open a discussion ticket, @webron may have an opinion 😄

Beyond that... I think I've ironed out the performance issues with the definition you've been looking at. Try this one, @heldersepu: https://halting-coat.surge.sh

heldersepu commented 6 years ago

Getting an error on that link: Failed to load API definition.

I just moved to a my laptop and testing my example, I noticed that the fan was kicking in right away... Triggering those UI events seem to really put a toll on Memory and CPU:

shockey commented 6 years ago

@heldersepu, try out https://halting-coat.surge.sh/?url=https://gist.githubusercontent.com/shockey/c62b2370ae3b93c61999158b78eb390b/raw/46e3f01ed576e34250bc6bf04b9e158b9db2b4ce/gistfile1.txt to load the spec you shared earlier.

From my end, the spec initially renders quite fast, and expanding each model takes 0-2 seconds of hard work on the CPU.

Doing the work at UI render time is the idea here, but we do want it to be performant. Let me know if your experience with that definition is very different from mine, or you have another that demonstrates a persisting problem!

heldersepu commented 6 years ago

Much Better! the browser does not crash https://halting-coat.surge.sh/?url=https://gist.githubusercontent.com/shockey/c62b2370ae3b93c61999158b78eb390b/raw/46e3f01ed576e34250bc6bf04b9e158b9db2b4ce/gistfile1.txt&filter=_&defaultModelsExpandDepth=5

Comparing Performance with my UI events idea this approach consumes significantly more memory.

heldersepu commented 6 years ago

halting-coat.surge.sh/.../gistfile1.txt&filter=_&defaultModelsExpandDepth=5 35 seconds to fully load

petstore.swagger.io/.../swaggerErliSoares.json&filter=&defaultModelsExpandDepth=5 47 seconds to fully load

Not a huge improvement... but I take it.!

You are not buying into the UI events idea, are you?

shockey commented 6 years ago

@heldersepu, yep, I'm not quite sold - I don't really see the use case.

As it stands: if you don't need everything on the page on load, then just expand things as you need them; if you do need everything visible, use docExpansion and defaultModelsExpandDepth, you'll have to wait but it'll all be there! Staggering it feels like an odd middle ground to land in, that has the same latency as the manual expansion but without the predictability of CPU cycles happening when the user interacts with the page.

That being said..... Swagger-UI core isn't the end-all-be-all. If you'd like to make a plugin for this functionality, check out my cornify plugin for a starting point.

shockey commented 6 years ago

Besides that.... Thanks for the discussion here today! The definition you shared really helped in speeding things up. Excited to ship the fastest Swagger-UI ever 😄

shockey commented 6 years ago

fyi, i marked this issue as fixed, but totally happy to keep the discussion going!

heldersepu commented 6 years ago

Keeping the discussion going...

With the "UI events idea" the page remains responsive while the models are expanding, with that in mind we could create something like the infinite scroll on the Google images search: https://www.google.com/search?q=images&source=lnms&tbm=isch Even if the user sets a high defaultModelsExpandDepth we do not need to expand all models right away, perhaps we can wait until the user scrolls down or follows a deep link to a model to do the expand.

Now about my sample definition, it is not mine and I don't remember who I got it from, but I do remember it was much larger, the original was over 1MB and I managed to reduce it to 40KB ... So there are developers out there with much larger & complex definitions

Also my test was with a conservative defaultModelsExpandDepth=5 if we double that the page will crash No loading animation, no please wait, just an unresponsive page for a while until the browser shows:

heldersepu commented 6 years ago

Looking at the current implementation, the collapsed models show right away: petstore.swagger.io/...&url=http://heldersepu.github.io/hs-scripts/swagger/4248_swagger.json That is a smaller version of the previous sample definition, but still 50 models.

Expanding small models is quick, but the largest models (such as Patrimonio ) takes quite a while, once it shows expanding child models takes no time... I believe that is because we are loading the entire tree of the model, right? Maybe lazy loading the model properties on the same way you did the Models is an alternative.

heldersepu commented 6 years ago

@shockey I'm going to create a new issue to improve performance of the model properties

Kevin-Prichard commented 6 years ago

@heldersepu, a slight modification to your code produces a single model version. Seldom do I want to open all models at once, especially with a complex API.

Connecting this code to a "toggle all" button next to the %gt; symbol for each model would enable us to manually open just one model entirely.

function ExpandModel(modelName) {
    const o = document.querySelectorAll('#model-'+modelName+" .model-toggle.collapsed")
    if (o.length > 0) {
        o[0].click()
        setTimeout(function(){ExpandModel(modelName)}, 100);
    }
}

ExpandModel("YourModelNameHere")

heldersepu commented 6 years ago

Hi @Kevin-Prichard, I'm glad my code was helpful... but the problem still exists, if we have a truly complex model it will crash, try with this: http://swagger-net-test.azurewebsites.net/api/NestedSwag/9

Use your ExpandModel to expand Test0 on that schema, let me know what happens

shayded-exe commented 6 years ago

I'm pretty sure this change is what broke our swagger in newer versions. We have a very large definition and rely on splitting up the paths into multiple files.

This used to work in v3.9:

paths:
  allOf:
    - $ref: 'common/errors.yaml#/paths'
    - $ref: 'common/versions.yaml#/paths'
    - $ref: 'users/users.yaml#/paths'
    # etc... there's nearly 30 files total

Now in the newer versions, swagger just says there's no operations defined. Is there any way to disable this lazy resolving behavior? Scanned through the comments and docs but I didn't see where an option had actually been implemented.

heldersepu commented 6 years ago

@PachowStudios

disable this lazy resolving behavior

Yes stick to the v3.9 that used to work for you. But If you have time create a minimal definition reproducing your problem and paste it here

shayded-exe commented 6 years ago

@heldersepu We can't be stuck on 3.9 forever. That isn't really a long term solution.

I've attached a minimal reproduction. If you downgrade to 3.9.2, it works fine. swagger-bug.zip

heldersepu commented 6 years ago

@PachowStudios For sure someone will look into your issue!

I was able to reproduce it: Old version is OK: http://offleaseonly.azurewebsites.net/swagger/ui/index?url=https://raw.githack.com/heldersepu/hs-scripts/master/swagger/4248/index.yaml

Latest shows No operations defined in spec! http://petstore.swagger.io/?url=https://raw.githack.com/heldersepu/hs-scripts/master/swagger/4248/index.yaml

You should probably open a new issue

swagger-api / swagger-ui

Lazy resolution #4248

Description

Plan