Skip to content

$ref resolution issues #1201

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 9, 2017
Merged

$ref resolution issues #1201

merged 4 commits into from
Dec 9, 2017

Conversation

@shockey
Copy link
Contributor

@shockey shockey commented Dec 9, 2017 •
edited
Loading

Description

This PR replaces the existing traversal cache with a result cache. The traversal cache was prone to issues that were causing display bugs in UI/Editor.

Motivation and Context

Fixes swagger-api/swagger-editor#1585. Fixes swagger-api/swagger-ui#4000.

Follow-up to #1190. It's not as performant, but it is more accurate.

How Has This Been Tested?

  • I added a unit test that was previously failing.
  • I linked the changes to Swagger-UI and observed that the display issues went away.

Types of changes

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@shockey
Copy link
Contributor Author

shockey commented Dec 9, 2017

cc @dballance: pretty sure this is a step back for performance (i'm caching the plugin result, not the traversal), so feel free to build on top of this.

@shockey shockey merged commit 8c21ccf into swagger-api:master Dec 9, 2017
@shockey shockey deleted the bug/editor-1585-refs-not-resolving branch December 9, 2017 03:57
@dballance
Copy link
Contributor

dballance commented Dec 9, 2017

I still need to make changes to allof to test my large spec, but the smaller one I tested in my PR showed an increase to ~580ms. Still close to half the original resolution time, but with greater accuracy - works for me. 👍

Will grab some results on the massive spec sometime soon. Have allOf work scheduled in January to try and resolve our issues....

shockey added a commit to shockey/swagger-js that referenced this pull request Dec 9, 2017
@shockey
Revert "Merge pull request swagger-api#1201 from shockey/bug/editor-1…
6bb60ca 
…585-refs-not-resolving"

This reverts commit 8c21ccf, reversing
changes made to 9155874.
shockey added a commit to shockey/swagger-js that referenced this pull request Dec 9, 2017
@shockey
Keep updated resolver tests from reverted PR swagger-api#1201
3903747
@shockey
Copy link
Contributor Author

shockey commented Dec 9, 2017

Well, this is embarrassing™

Turns out this PR wasn't passing all tests - I left a describe.only in the tests that kept.... all the tests, from running.

I've reverted this PR, so we're back to no optimizations for now - I thought it better to ship unoptimized than to hold up the release.

I'll circle back here on Monday to get this in full working order 😄

@shockey shockey mentioned this pull request Dec 9, 2017
Merged
@dballance dballance mentioned this pull request Dec 19, 2017
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

2 participants

@shockey @dballance