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

Warns when mutated props are passed. #5346

Merged

Conversation

Projects
None yet
5 participants
@prometheansacrifice
Copy link
Contributor

commented Oct 31, 2015

Attempts for fix #5335

@@ -147,6 +147,7 @@ var ReactCompositeComponentMixin = {
// Initialize the public class
var inst;
var renderedElement;
var propsMuted = false;

This comment has been minimized.

Copy link
@chicoxyzzy

chicoxyzzy Oct 31, 2015

Contributor

did you mean mutated?

if (instPropsKeys.length !== pubPropsKeys.length) {
arePropsMuted = true;
}
for (i = 0, ii = instPropsKeys.length; i < ii; i += 1) {

This comment has been minimized.

Copy link
@chicoxyzzy

chicoxyzzy Oct 31, 2015

Contributor

you can use shallowEqual here

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-immutable-props branch from 17104f0 to ef6aa1e Oct 31, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 31, 2015

@prometheansacrifice updated the pull request.

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-immutable-props branch from ef6aa1e to 45a49a0 Nov 2, 2015

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2015

@jimfb Good enough?

if (propsMutated) {
warning(
propsMutated,
'Mutated props not allowed in %s(...)',

This comment has been minimized.

Copy link
@jimfb

jimfb Nov 3, 2015

Contributor

If someone is debugging, just saying "Mutated props not allowed in..." doesn't really describe what's wrong. Someone who is writing a moderately complex component might not see what the issue is.

I would say something like "Constructor (of %s) may not invoke super(...) with props that differ from the props that were initially passed in from the component's owner."

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 3, 2015

Author Contributor

Agreed.

@@ -195,6 +197,15 @@ var ReactCompositeComponentMixin = {
Component.displayName || Component.name || 'Component'
);
}

propsMutated = !shallowEqual(inst.props, publicProps);

This comment has been minimized.

Copy link
@jimfb

jimfb Nov 3, 2015

Contributor

Nit: Move the "var" down here to constrict the scope of propsMutated.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 3, 2015

Author Contributor

Why? Variables in Javascript are known to have function scope.

This comment has been minimized.

Copy link
@jimfb

jimfb Nov 3, 2015

Contributor

Because it's more inline with our facebook style, it gives linters an opportunity to catch more errors, and it minimizes the number of variables that a developer needs to keep in their mental working set.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 3, 2015

Author Contributor

Alright

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 5, 2015

Collaborator

This should use === not shallowEqual for performance (and better semantics, IMO) and this check should be dev-only.

This comment has been minimized.

Copy link
@jimfb

jimfb Nov 12, 2015

Contributor

Perf was my initial reaction too, but if I understand this code correctly, that check is already dev only.

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 12, 2015

Collaborator

Yes, but we still shouldn't make things slower than necessary.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2015

Also, are you sure this works? I thought we set inst.props to always be publicProps (the diff that proposed changing this is what spawned the whole discussion). Anyway, we should add a unit test to assert the warning fires as expected.

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-immutable-props branch from 45a49a0 to b05ffdd Nov 4, 2015

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2015

@jimfb Warning does show for the component. However, it is also triggered for another component TopLevelWrapper everywhere. This leads to all warning related tests to fail.

What is TopLevelWrapper for anyways?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

@prometheansacrifice TopLevelWrapper is created at the top/root of each render tree. It is a temporary (?) hack so that we can store all top-level pending updates on composites instead of having to worry about different types of components.

I'm a little surprised that TopLevelWrapper warns, since I wouldn't expect it to be doing anything fancy with props. The entire definition (in ReactMount.js) is literally only a few lines:

var TopLevelWrapper = function() {};
TopLevelWrapper.prototype.isReactComponent = {};
if (__DEV__) {
  TopLevelWrapper.displayName = 'TopLevelWrapper';
}
TopLevelWrapper.prototype.render = function() {
  // this.props is actually a ReactElement
  return this.props;
};  

Anyway, we should fix so it doesn't warn for TopLevelWrapper.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 5, 2015

@prometheansacrifice updated the pull request.

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Nov 5, 2015

Let's also not warn if you pass up undefined (e.g., super();). In examples we've indicated that you only need to pass up props if you want to access this.props in the constructor (same with context as a second arg).

return <span />;
};

function Bar (props) {

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 5, 2015

Collaborator

Do you need Foo at all? Why can't Bar extend React.Component directly?

Also, can you use ES6 class syntax here? It's simpler.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 5, 2015

Author Contributor

Sure. Foo (or Bar) can directly inherit. Making the change in the next commit.

Is ES6 necessary here? I find most of the React Component definition is ES5 syntax. Simpler as it maybe, mixing up the syntax looks bad on the file as a whole.

This comment has been minimized.

Copy link
@jimfb

jimfb Nov 5, 2015

Contributor

We're slowly transitioning to ES6. You'll notice that ES6 syntaxes have been appearing in the docs too; most of the new stuff uses ES6.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 5, 2015

Author Contributor

Alright then.

if (propsMutated) {
warning(
!propsMutated,
'Constructor (of %s) may not invoke super(...) with props that ' +

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 5, 2015

Collaborator

This error message is difficult to understand. How about:

%s(...): When calling super() in `%s`, make sure to pass up the same props that your component's constructor was passed.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 5, 2015

Author Contributor

@jimfb @spicyj Can we have a consensus over the warning message? I personally find both the messages good enough.

This comment has been minimized.

Copy link
@jimfb

jimfb Nov 5, 2015

Contributor

I'm fine with @spicyj's suggestion.

@prometheansacrifice prometheansacrifice changed the title Warns when muted props are passed. Warns when mutated props are passed. Nov 5, 2015

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2015

@jimfb Is the behaviour correct in the first place? Is it okay for TopLevelWrapper to pass mutated props to super() ? Or should I ignore it in the test with a Component.displayName !== 'TopLevelWrapper'

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Nov 7, 2015

TopLevelWrapper doesn't call super at all (which is allowed). Look at its definition in ReactMount. You should not ignore it with a check for its displayName.

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-immutable-props branch from b05ffdd to 04e9539 Nov 12, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 12, 2015

@prometheansacrifice updated the pull request.

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-immutable-props branch from 04e9539 to 8a9b3df Nov 12, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 12, 2015

@prometheansacrifice updated the pull request.

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-immutable-props branch from 8a9b3df to 7a86969 Nov 12, 2015

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2015

@spicyj Added the fix. Can you please review once again?

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 12, 2015

@prometheansacrifice updated the pull request.

@@ -195,6 +196,18 @@ var ReactCompositeComponentMixin = {
Component.displayName || Component.name || 'Component'
);
}

var propsMutated = !shallowEqual(inst.props || {}, publicProps);

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 12, 2015

Collaborator

This must use ===, not shallowEqual.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 12, 2015

Author Contributor

By ===, you mean deep comparison of objects? I remember @jimfb asking me to avoid as it is expensive.

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 12, 2015

Collaborator

=== just checks if they are the same reference; it is less expensive than shallowEqual.

@@ -195,6 +196,18 @@ var ReactCompositeComponentMixin = {
Component.displayName || Component.name || 'Component'
);
}

var propsMutated = !shallowEqual(inst.props || {}, publicProps);
var componentName = Component.displayName || Component.name ||

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 12, 2015

Collaborator

Can you break after the = (and indent the second line only 2 spaces) instead?

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 12, 2015

Author Contributor

Like this?

      var propsMutated =
        !shallowEqual(inst.props || {}, publicProps);
      var componentName =
        Component.displayName || Component.name ||

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 12, 2015

Collaborator

Only if the line doesn't fit on 80 chars, but yes.

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 12, 2015

Author Contributor

Currently its just 70 characters. Still break after =?

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 12, 2015

Collaborator

Yes, we prefer

      var componentName =
        Component.displayName || Component.name || 'Component';

to

      var componentName = Component.displayName || Component.name ||
            'Component';

(since it doesn't fit all on one line).

'Component';

warning(
typeof inst.props === 'undefined' ||

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 12, 2015

Collaborator

inst.props === undefined is simpler – but why not do this check in the React.Component constructor instead?

This comment has been minimized.

Copy link
@prometheansacrifice

prometheansacrifice Nov 12, 2015

Author Contributor

The same would have to be repeated at React.createClass right?

@prometheansacrifice prometheansacrifice force-pushed the prometheansacrifice:warn-immutable-props branch from 7a86969 to 593a234 Nov 12, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Nov 12, 2015

@prometheansacrifice updated the pull request.

@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2015

@jimfb @spicyj Does this PR need more work?

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Dec 17, 2015

@prometheansacrifice Sorry for the delay. This seems good – thank you.

sophiebits added a commit that referenced this pull request Dec 17, 2015

@sophiebits sophiebits merged commit 963b3ca into facebook:master Dec 17, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@prometheansacrifice

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2015

@spicyj Thank you too :)

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.