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.

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

Do not clone key and ref getter props #6268

Closed
wants to merge 1 commit into from
Closed

Do not clone key and ref getter props #6268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions src/isomorphic/classic/element/ReactElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,19 @@ var RESERVED_PROPS = {

var specialPropKeyWarningShown, specialPropRefWarningShown;

function isValidConfigRefOrKey(config, name) {
if (__DEV__) {
return config.hasOwnProperty(name) &&
!Object.getOwnPropertyDescriptor(config, name).get;
}

return config[name] !== undefined;
}

function getConfigKey(config) {
return '' + config.key;
}

/**
* Factory method to create a new React element. This no longer adheres to
* the class pattern, so do not use new to call it. Also, no instanceof check
Expand Down Expand Up @@ -133,14 +146,16 @@ ReactElement.createElement = function(type, config, children) {
'React.createElement(...): Expected props argument to be a plain object. ' +
'Properties defined in its prototype chain will be ignored.'
);
ref = !config.hasOwnProperty('ref') ||
Object.getOwnPropertyDescriptor(config, 'ref').get ? null : config.ref;
key = !config.hasOwnProperty('key') ||
Object.getOwnPropertyDescriptor(config, 'key').get ? null : '' + config.key;
} else {
ref = config.ref === undefined ? null : config.ref;
key = config.key === undefined ? null : '' + config.key;
}

if (isValidConfigRefOrKey(config, 'ref')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this indirection (passing 'ref' instead of directly reading config.ref) cause any performance regressions in the production path? I’m not well versed in optimizations like this but it’s a super hot code path so we better make sure we don’t regress on it in prod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're adding potentially 3 function calls. It could very easily be reduced to 2 since getConfigKey isn't very necessary. The function has the same undefined condition that existed previously, but it now skips assigning null again since the value is already null. That doesn't seem like enough to make any measurable difference in performance, but I'm sure if you have any thoughts on proving that empirically.

ref = config.ref;
}

if (isValidConfigRefOrKey(config, 'key')) {
key = getConfigKey(config);
}

self = config.__self === undefined ? null : config.__self;
source = config.__source === undefined ? null : config.__source;
// Remaining properties are added to a new props object
Expand Down Expand Up @@ -284,14 +299,17 @@ ReactElement.cloneElement = function(element, config, children) {
'Properties defined in its prototype chain will be ignored.'
);
}
if (config.ref !== undefined) {

if (isValidConfigRefOrKey(config, 'ref')) {
// Silently steal the ref from the parent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any specific reason why you're changing comparisons to be looser here? They used to be true for null but are now false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The configRef value is returned by getRef and can be null. We could strictly check null, but an undefined or null check felt safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If null values should be allowed here, I think we'd need to check config.ref and config.key before calling getRef and getKey and then always set ref and key to the return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allowing key to be null seems like a mistake, since it is coerced to the string "null".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this approach should keep behavior consistent. Let me know what you think and I'll add a test for the null cases.

function isValidConfigRefOrKey(config, name) {
  if (__DEV__) {
    return config.hasOwnProperty(name) &&
      !Object.getOwnPropertyDescriptor(config, name).get;
  }

  return config[name] !== undefined;
}

function getConfigKey(config) {
  return '' + config.key;
}

...

ReactElement.createElement = function(type, config, children) {
  ...

    if (isValidConfigRefOrKey(config, 'ref')) {
      ref = config.ref;
    }

    if (isValidConfigRefOrKey(config, 'key')) {
      key = getConfigKey(config);
    }

...

ReactElement.cloneElement = function(element, config, children) {
  ...

    if (isValidConfigRefOrKey(config, 'ref')) {
      // Silently steal the ref from the parent.
      ref = config.ref;
      owner = ReactCurrentOwner.current;
    }

    if (isValidConfigRefOrKey(config, 'key')) {
      key = getConfigKey(config);
    }

ref = config.ref;
owner = ReactCurrentOwner.current;
}
if (config.key !== undefined) {
key = '' + config.key;

if (isValidConfigRefOrKey(config, 'key')) {
key = getConfigKey(config);
}

// Remaining properties override existing props
var defaultProps;
if (element.type && element.type.defaultProps) {
Expand Down
80 changes: 80 additions & 0 deletions src/isomorphic/classic/element/__tests__/ReactElement-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,86 @@ describe('ReactElement', function() {
expect(element.props).toEqual(expectation);
});

it('should not extract key and ref getters from the config when creating an element', function() {
var props = {
foo: '56',
};

Object.defineProperty(props, 'key', {
get: function() {
return '12';
},
});

Object.defineProperty(props, 'ref', {
get: function() {
return '34';
},
});

var element = React.createFactory(ComponentClass)(props);
expect(element.type).toBe(ComponentClass);
expect(element.key).toBe(null);
expect(element.ref).toBe(null);
var expectation = {foo:'56'};
Object.freeze(expectation);
expect(element.props).toEqual(expectation);
});

it('should not extract key and ref getters from the config when cloning an element', function() {
var element = React.createFactory(ComponentClass)({
key: '12',
ref: '34',
foo: '56',
});

var props = {
foo: 'ef',
};

Object.defineProperty(props, 'key', {
get: function() {
return 'ab';
},
});

Object.defineProperty(props, 'ref', {
get: function() {
return 'cd';
},
});

var clone = React.cloneElement(element, props);
expect(clone.type).toBe(ComponentClass);
expect(clone.key).toBe('12');
expect(clone.ref).toBe('34');
var expectation = {foo:'ef'};
Object.freeze(expectation);
expect(clone.props).toEqual(expectation);
});

it('should allow null key and ref values when cloning an element', function() {
var element = React.createFactory(ComponentClass)({
key: '12',
ref: '34',
foo: '56',
});

var props = {
key: null,
ref: null,
foo: 'ef',
};

var clone = React.cloneElement(element, props);
expect(clone.type).toBe(ComponentClass);
expect(clone.key).toBe('null');
expect(clone.ref).toBe(null);
var expectation = {foo:'ef'};
Object.freeze(expectation);
expect(clone.props).toEqual(expectation);
});

it('coerces the key to a string', function() {
var element = React.createFactory(ComponentClass)({
key: 12,
Expand Down