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

Don't define key or ref dummy props if none were provided #7488

Merged
merged 2 commits into from
Aug 13, 2016
Merged

Don't define key or ref dummy props if none were provided #7488

merged 2 commits into from
Aug 13, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 13, 2016

This fixes another DEV-only performance regression introduced in 15.x.

The idea here is that extra defineProperty getters are slow. Let’s only add them when we know user supplied key (or ref). Otherwise the user wouldn’t expect to find them in props anyway.

Chrome (before):

create 10k rows: 6596
update 1k rows: 1447
update 1k rows: 1362
update 1k rows: 1385
clear 10k rows: 1094

create 10k rows: 6403
update 1k rows: 1428
update 1k rows: 1321
update 1k rows: 1393
clear 10k rows: 1182

Chrome (after):

create 10k rows: 5373
update 1k rows: 1294
update 1k rows: 1313
update 1k rows: 1246
clear 10k rows: 1060

create 10k rows: 5446
update 1k rows: 1326
update 1k rows: 1233
update 1k rows: 1268
clear 10k rows: 1102

Firefox (before):

create 10k rows: 10431
update 1k rows: 1664
update 1k rows: 896
update 1k rows: 853
clear 10k rows: 4157

create 10k rows: 10044
update 1k rows: 1091
update 1k rows: 1197
update 1k rows: 847
clear 10k rows: 3225

Firefox (after):

create 10k rows: 8241
update 1k rows: 858
update 1k rows: 715
update 1k rows: 725
clear 10k rows: 3877

create 10k rows: 8200
update 1k rows: 831
update 1k rows: 775
update 1k rows: 720
clear 10k rows: 4239

I’m too lazy to format Safari results but they show similar improvements although smaller.

displayName
);
}
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it useful to return undefined; as the last line of a function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copypasta. Not sure why it was there. Removing.

@ghost ghost added the CLA Signed label Aug 13, 2016
@sophiebits
Copy link
Collaborator

Seems fine. I wonder if it makes more sense to put these on a prototype and have the props object extend from there. That would subtly change some semantics though so maybe this is better.

@ghost ghost added the CLA Signed label Aug 13, 2016
@gaearon gaearon merged commit a56e105 into facebook:master Aug 13, 2016
@zpao zpao modified the milestones: 15.3.1, 15-next Aug 15, 2016
zpao pushed a commit that referenced this pull request Aug 15, 2016
* Don't define key or ref dummy props if none were provided

This fixes a performance regression.

* Style nit

(cherry picked from commit a56e105)
@ghengeveld
Copy link
Contributor

ghengeveld commented Aug 30, 2016

My unit tests for react-isomorphic-form are failing since this change. I see that apparently I'm not supposed to be using this.props.ref, but I am. Bare example:

class Input extends React.Component {
  constructor(props) {
    super(props)
    this.ref = this.ref.bind(this)
  }
  ref(el) {
    // do stuff
    this.props.ref(el)
  }
  render() {
    return <input {...this.props} ref={this.ref} />
  }
}

Full source: https://github.com/ghengeveld/react-isomorphic-form/blob/master/src/Input.js

I think dealing with ref that way makes perfect sense and allows for a more predictable API. I'm trying to create a component that is a very thin wrapper around a native HTML component, so I don't want to use custom prop names. Perhaps I should switch to higher-order components but I'm not sure that would provide a nicer or clearer API.

Suggestions are welcome.

@sophiebits
Copy link
Collaborator

this.props.ref will always be undefined for you, so your example does not work as you intend. #4213 tracks supporting your use case.

@gaearon gaearon deleted the key-ref-regression branch August 30, 2016 19:56
@ghengeveld
Copy link
Contributor

This worked fine in 15.3.0. For now I'll just remove the ability to pass through ref as a prop, awaiting #4213.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 31, 2016

It worked fine by accident because we defined a dummy ref prop in development that just printed a warning. It was never part of public API and not mean to be supported.

If you are sure it’s our mistake please provide a jsfiddle showing the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants