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

Warn when a DOM element has unsupported property #6465

Closed

Conversation

nhunzaker
Copy link
Contributor

This commit updates the logic in ReactDOMUnknownPropertyDevTool such that it will warn when a DOM property not contained in the property whitelist is assigned to a React DOM element.

Basically a follow up to @jimfb's suggestion in #6459.

This commit updates the logic in ReactDOMUnknownPropertyDevTool
such that it will warn when a DOM property not contained in the property
whitelist is assigned to a React DOM element.
standardName != null || registrationName != null,
'Unable to assign unsupported DOM property %s.',
name
);
};
}
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 updated the warning copy here to make it clearer what's going on. Happy to change this.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that this error message doesn't specify the line number, as per the last sentence of my comment in #6459 (comment)

@gaearon
Copy link
Collaborator

gaearon commented Apr 11, 2016

Hmm. I think the plan was to move these warnings to element creation: #6345

Never mind, I meant to comment on a different pull request.

@nhunzaker
Copy link
Contributor Author

@jimfb given my short-sight on the spread operator, and outstanding work in #6398, I may have been hasty sending this out. Should this be closed out?

@jimfb
Copy link
Contributor

jimfb commented Apr 13, 2016

@nhunzaker: It's up to you. #6398 does solve a subtly different problem and hasn't been touched in almost a week, but this commit is going to depend on some incarnation of that PR so it will be tough to make progress on this until that gets finished. You might be able to adopt that PR (eg. merge it into here), fix it up, and the two of you can work together to get it merged. Up to you guys to figure out how to best proceed.

Fair warning, #6398 may be a rough one to attempt as a first bug, as per #6062 (comment). I can offer my guidance on this bug, but ultimately the team would need to arrive at a consensus which means that implementation/design decisions would be up for debate, which can be a little frustrating for outside contributors. If you're looking for an easier way to get involved, I recommend looking at https://github.com/facebook/react/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+bug%22 . Anyway, your call. This one would certainly be a valuable one to get solved!

@nhunzaker
Copy link
Contributor Author

@jimfb Sorry to keep this out so long. Looks like #6800 addresses this? I'm going to close this out.

@nhunzaker nhunzaker closed this Jul 1, 2016
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.

4 participants