Skip to content

optimize compat by hoisting the vnode handling func #3941

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

Merged
merged 4 commits into from
Mar 22, 2023
Merged

optimize compat by hoisting the vnode handling func #3941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a6399d
optimize compat by hoisting the vnode handling func
JoviDeCroock Mar 20, 2023
bbfea5c
fix
JoviDeCroock Mar 20, 2023
d9abd68
optimise className handling
JoviDeCroock Mar 20, 2023
9658dcf
add todo
JoviDeCroock Mar 20, 2023
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
192 changes: 102 additions & 90 deletions compat/src/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,113 +101,125 @@ function isDefaultPrevented() {
return this.defaultPrevented;
}

let classNameDescriptor = {
const classNameDescriptorNonEnumberable = {
enumerable: false,
configurable: true,
get() {
return this.class;
}
};

let oldVNodeHook = options.vnode;
options.vnode = vnode => {
let type = vnode.type;
let props = vnode.props;
let normalizedProps = props;

// only normalize props on Element nodes
if (typeof type === 'string') {
function handleDomVNode(vnode) {
let props = vnode.props,
type = vnode.type,
normalizedProps = {};

for (let i in props) {
let value = props[i];

if (
(i === 'value' && 'defaultValue' in props && value == null) ||
// Emulate React's behavior of not rendering the contents of noscript tags on the client.
(IS_DOM && i === 'children' && type === 'noscript')
) {
// Skip applying value if it is null/undefined and we already set
// a default value
continue;
}

let lowerCased = i.toLowerCase();
if (i === 'defaultValue' && 'value' in props && props.value == null) {
// `defaultValue` is treated as a fallback `value` when a value prop is present but null/undefined.
// `defaultValue` for Elements with no value prop is the same as the DOM defaultValue property.
i = 'value';
} else if (i === 'download' && value === true) {
// Calling `setAttribute` with a truthy value will lead to it being
// passed as a stringified value, e.g. `download="true"`. React
// converts it to an empty string instead, otherwise the attribute
// value will be used as the file name and the file will be called
// "true" upon downloading it.
value = '';
} else if (lowerCased === 'ondoubleclick') {
i = 'ondblclick';
} else if (
lowerCased === 'onchange' &&
(type === 'input' || type === 'textarea') &&
!onChangeInputType(props.type)
) {
lowerCased = i = 'oninput';
} else if (lowerCased === 'onfocus') {
i = 'onfocusin';
} else if (lowerCased === 'onblur') {
i = 'onfocusout';
} else if (ON_ANI.test(i)) {
i = lowerCased;
} else if (type.indexOf('-') === -1 && CAMEL_PROPS.test(i)) {
i = i.replace(CAMEL_REPLACE, '-$&').toLowerCase();
} else if (value === null) {
value = undefined;
}

// Add support for onInput and onChange, see #3561
// if we have an oninput prop already change it to oninputCapture
if (lowerCased === 'oninput') {
i = lowerCased;
if (normalizedProps[i]) {
i = 'oninputCapture';
}
}
for (let i in props) {
let value = props[i];

normalizedProps[i] = value;
if (
(i === 'value' && 'defaultValue' in props && value == null) ||
// Emulate React's behavior of not rendering the contents of noscript tags on the client.
(IS_DOM && i === 'children' && type === 'noscript') ||
i === 'class' ||
i === 'className'
) {
// Skip applying value if it is null/undefined and we already set
// a default value
continue;
}

// Add support for array select values: <select multiple value={[]} />
if (
type == 'select' &&
normalizedProps.multiple &&
Array.isArray(normalizedProps.value)
let lowerCased = i.toLowerCase();
if (i === 'defaultValue' && 'value' in props && props.value == null) {
// `defaultValue` is treated as a fallback `value` when a value prop is present but null/undefined.
// `defaultValue` for Elements with no value prop is the same as the DOM defaultValue property.
i = 'value';
} else if (i === 'download' && value === true) {
// Calling `setAttribute` with a truthy value will lead to it being
// passed as a stringified value, e.g. `download="true"`. React
// converts it to an empty string instead, otherwise the attribute
// value will be used as the file name and the file will be called
// "true" upon downloading it.
value = '';
} else if (lowerCased === 'ondoubleclick') {
i = 'ondblclick';
} else if (
lowerCased === 'onchange' &&
(type === 'input' || type === 'textarea') &&
!onChangeInputType(props.type)
) {
// forEach() always returns undefined, which we abuse here to unset the value prop.
normalizedProps.value = toChildArray(props.children).forEach(child => {
child.props.selected =
normalizedProps.value.indexOf(child.props.value) != -1;
});
lowerCased = i = 'oninput';
} else if (lowerCased === 'onfocus') {
i = 'onfocusin';
} else if (lowerCased === 'onblur') {
i = 'onfocusout';
} else if (ON_ANI.test(i)) {
i = lowerCased;
} else if (type.indexOf('-') === -1 && CAMEL_PROPS.test(i)) {
i = i.replace(CAMEL_REPLACE, '-$&').toLowerCase();
} else if (value === null) {
value = undefined;
}

// Adding support for defaultValue in select tag
if (type == 'select' && normalizedProps.defaultValue != null) {
normalizedProps.value = toChildArray(props.children).forEach(child => {
if (normalizedProps.multiple) {
child.props.selected =
normalizedProps.defaultValue.indexOf(child.props.value) != -1;
} else {
child.props.selected =
normalizedProps.defaultValue == child.props.value;
}
});
// Add support for onInput and onChange, see #3561
// if we have an oninput prop already change it to oninputCapture
if (lowerCased === 'oninput') {
i = lowerCased;
if (normalizedProps[i]) {
i = 'oninputCapture';
}
}

vnode.props = normalizedProps;
normalizedProps[i] = value;
}

// Add support for array select values: <select multiple value={[]} />
if (
type == 'select' &&
normalizedProps.multiple &&
Array.isArray(normalizedProps.value)
) {
// forEach() always returns undefined, which we abuse here to unset the value prop.
normalizedProps.value = toChildArray(props.children).forEach(child => {
child.props.selected =
normalizedProps.value.indexOf(child.props.value) != -1;
});
}

if (props.class != props.className) {
Copy link
Member Author

Choose a reason for hiding this comment

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

What happened before:

  • props.class === undefined && props.className !== undefined --> we would make className enumerable and make class === className
  • props.class !== undefined && props.className === undefined --> we would make className not enumerable and make className return props.class
  • props.class !== undefined && props.className !== undefined --> we would make className enumerable and make class === className

Position 1 and 3 are different because we are just returning the value for className rather than the one for class, I don't think this matters.
Position 2 is different because we aren't introducing a getter for className to return class, I don't think this matters

CC @marvinhagemeister

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking. That sounds reasonable. I'm good with merging the PR 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yep that seems reasonable to me. I think the dynamic assignment of enumerable here was from when we used to make class a descriptor too.

classNameDescriptor.enumerable = 'className' in props;
if (props.className != null) normalizedProps.class = props.className;
Object.defineProperty(normalizedProps, 'className', classNameDescriptor);
}
// Adding support for defaultValue in select tag
if (type == 'select' && normalizedProps.defaultValue != null) {
normalizedProps.value = toChildArray(props.children).forEach(child => {
if (normalizedProps.multiple) {
child.props.selected =
normalizedProps.defaultValue.indexOf(child.props.value) != -1;
} else {
child.props.selected =
normalizedProps.defaultValue == child.props.value;
}
});
}

if (props.class && !props.className) {
normalizedProps.class = props.class;
Object.defineProperty(
normalizedProps,
'className',
classNameDescriptorNonEnumberable
);
} else if (props.className && !props.class) {
normalizedProps.class = normalizedProps.className = props.className;
} else if (props.class && props.className) {
normalizedProps.class = normalizedProps.className = props.className;
}

vnode.props = normalizedProps;
}

let oldVNodeHook = options.vnode;
options.vnode = vnode => {
// only normalize props on Element nodes
if (typeof vnode.type === 'string') {
handleDomVNode(vnode);
}

vnode.$$typeof = REACT_ELEMENT_TYPE;
Expand Down
2 changes: 2 additions & 0 deletions compat/test/browser/render.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ describe('compat render', () => {
it('should preserve className, add class alias', () => {
const { props } = <ul className="from className" />;
expect(props).to.have.property('className', 'from className');
// TODO: why would we do this, assuming that folks add className themselves
expect(props).to.have.property('class', 'from className');
});

Expand All @@ -342,6 +343,7 @@ describe('compat render', () => {
const { props } = <ul className="from className" />;
const spreaded = (<li a {...props} />).props;
expect(spreaded).to.have.property('className', 'from className');
// TODO: why would we do this, assuming that folks add className themselves
expect(spreaded).to.have.property('class', 'from className');
expect(spreaded.propertyIsEnumerable('class')).to.equal(true);
});
Expand Down