Skip to content

Dedupe owner-is-important warning.#3145

Merged
zpao merged 1 commit into
facebook:masterfrom
jimfb:dedup-is-owner-important
Feb 14, 2015
Merged

Dedupe owner-is-important warning.#3145
zpao merged 1 commit into
facebook:masterfrom
jimfb:dedup-is-owner-important

Conversation

@jimfb
Copy link
Copy Markdown
Contributor

@jimfb jimfb commented Feb 14, 2015

Dedupe owner-is-important warning.

@jimfb
Copy link
Copy Markdown
Contributor Author

jimfb commented Feb 14, 2015

cc @zpao @sebmarkbage

Comment thread src/core/shouldUpdateReactComponent.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine with above conditional? The only content in this block is the if. Do we foresee anything else coming in here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are independent concerns, so I'm thinking they should be independent if statements. Especially with the newer if statement (checking both owners) which is more complicated to reason about.

@jimfb jimfb force-pushed the dedup-is-owner-important branch 2 times, most recently from 0b57837 to ddd77d8 Compare February 14, 2015 01:39
Comment thread src/core/shouldUpdateReactComponent.js Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are fixable line-length warnings. Can you fix them?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rrrarw, yeah, ok. This is a perfect example of why the line length warning SUCKS and should be removed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably this is a perfect example of why you should avoid statement nesting. :)

On Feb 13, 2015, at 6:00 PM, Jim notifications@github.com wrote:

In src/core/shouldUpdateReactComponent.js:

  •              (nextElement._owner != null &&
    
  •              nextElement._owner._isOwnerNecessary === false)) {
    
  •            if (prevElement._owner != null) {
    
  •              prevElement._owner._isOwnerNecessary = true;
    
  •            }
    
  •            if (nextElement._owner != null) {
    
  •              nextElement._owner._isOwnerNecessary = true;
    
  •            }
    
  •            warning(
    
  •              false,
    
  •              '<%s /> is being rendered by both %s and %s using the same key ' +
    
  •              '(%s) in the same place. Currently, this means that they ' +
    
  •              'don\'t preserve state. This behavior should be very rare ' +
    
  •              'so we\'re considering deprecating it. Please contact the ' +
    
  •              'React team and explain your use case so that we can take that ' +
    
  •              'into consideration.',
    
    Rrrarw, yeah, ok. This is a perfect example of why the line length warning SUCKS and should be removed.


Reply to this email directly or view it on GitHub.

@jimfb jimfb force-pushed the dedup-is-owner-important branch from ddd77d8 to 0e53889 Compare February 14, 2015 02:03
@sebmarkbage
Copy link
Copy Markdown
Contributor

Cool. Looking forward to testing this internally to see what it does to the stats.

zpao added a commit that referenced this pull request Feb 14, 2015
@zpao zpao merged commit ad01f21 into facebook:master Feb 14, 2015
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 participants