Skip to content

Conversation

@shockey
Copy link
Contributor

@shockey shockey commented Aug 5, 2017

This is a hotfix release that fixes #3527.

This master->feature merge commit that I made yesterday when merging #3509 contained a subtle error.

What happened

In ObjectModel, we destructure props at the top of render(), like we do in many components:

let { schema, name, isRef, getComponent, depth, expandDepth, ...props } = this.props

There's two things going on here: we're grabbing some props from this.props that we want and adding them to the local scope, and storing the rest of the props in props with destructuring rest syntax.

In the merge commit linked above, I added specSelectors to the destructuring statement, to support the object model anyOf/oneOf/not changes from #3513. Here's the props destructuring statement after the merge:

let { schema, name, isRef, getComponent, depth, expandDepth, specSelectors, ...props } = this.props

Further down in the same render function, we pass props to child components (which end up being the right-hand side of the Model display, seen broken in the issue linked above):

<Model 
  { ...props } // this is the important bit
  required={ false }
  getComponent={ getComponent }
  schema={ additionalProperties }
  depth={ depth + 1 }
 />

We're spreading our props object onto the child component with { ...props }.

Unfortunately, my merge commit that added specSelectors to the props destructuring statement (for use in the ObjectModel scope) had the side effect of removing specSelectors from the props object used here. This resulted in Model having no specSelectors prop, which it needs to work properly, especially with the OAS3 deprecated parameter support introduced this week in #3458.


There was no linter or test indication that something had gone wrong. I know this is a bit of a "perfect storm" situation, but this practice of using "rest of props" is fragile.

I propose we end use of rest syntax in props destructuring by implementing an ESLint rule. Instead, we should be verbose by creating objects specifically for use in spreading onto child props, or simply passing in each prop explicitly to the child.

P.S.: It's worth noting that #3265 would have saved us from shipping this.

@shockey shockey merged commit 93f1f65 into swagger-api:master Aug 5, 2017
@shockey shockey mentioned this pull request Aug 5, 2017
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.

3.1.3 broke rendering of model properties

1 participant