Skip to content

Use object bracket access test for RESERVED_PROPS #12530

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

Closed
Deraen wants to merge 1 commit into from
Closed

Use object bracket access test for RESERVED_PROPS #12530

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
10 changes: 5 additions & 5 deletions packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,10 +310,10 @@ function processContext(type, context) {

const STYLE = 'style';
const RESERVED_PROPS = {
children: null,
dangerouslySetInnerHTML: null,
suppressContentEditableWarning: null,
suppressHydrationWarning: null,
children: true,
dangerouslySetInnerHTML: true,
suppressContentEditableWarning: true,
suppressHydrationWarning: true,
};

function createOpenTagMarkup(
Expand All @@ -339,7 +339,7 @@ function createOpenTagMarkup(
}
let markup = null;
if (isCustomComponent(tagLowercase, props)) {
if (!RESERVED_PROPS.hasOwnProperty(propKey)) {
if (!RESERVED_PROPS[propKey]) {
Copy link
Copy Markdown
Collaborator

@gaearon gaearon Apr 3, 2018

Choose a reason for hiding this comment

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

I feel hesitant because there might've been some reason it was written this way. For example I think this will have a false positive for hasOwnProperty. And potentially will make adding anything to Object prototype in JS spec a breaking change.

Copy link
Copy Markdown
Author

@Deraen Deraen Apr 3, 2018

Choose a reason for hiding this comment

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

Oh right. I forgot how JS objects work.

I think I'm out of ideas for fixing #12368 here. I'll check on Closure side if it can be made to understand hasOwnProperty as dynamic access so it can keep the properties like with bracket access.

Copy link
Copy Markdown
Collaborator

@gaearon gaearon Apr 3, 2018

Choose a reason for hiding this comment

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

Could you give me more details on your strategy in general? Is this the only issue blocking you? It's a bit hard to believe given in how many places we use dot-access.

markup = createMarkupForCustomAttribute(propKey, propValue);
}
} else {
Expand Down
10 changes: 2 additions & 8 deletions packages/react/src/ReactElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,7 @@ export function createElement(type, config, children) {
source = config.__source === undefined ? null : config.__source;
// Remaining properties are added to a new props object
for (propName in config) {
if (
hasOwnProperty.call(config, propName) &&
!RESERVED_PROPS.hasOwnProperty(propName)
) {
if (hasOwnProperty.call(config, propName) && !RESERVED_PROPS[propName]) {
props[propName] = config[propName];
}
}
Expand Down Expand Up @@ -324,10 +321,7 @@ export function cloneElement(element, config, children) {
defaultProps = element.type.defaultProps;
}
for (propName in config) {
if (
hasOwnProperty.call(config, propName) &&
!RESERVED_PROPS.hasOwnProperty(propName)
) {
if (hasOwnProperty.call(config, propName) && !RESERVED_PROPS[propName]) {
if (config[propName] === undefined && defaultProps !== undefined) {
// Resolve default props
props[propName] = defaultProps[propName];
Expand Down