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

Remove enableFilterEmptyStringAttributesDOM #31765

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Remove enableFilterEmptyStringAttributesDOM #31765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d96eecb
Remove enableComponentStackLocations
rickhanlonii Dec 13, 2024
995beaf
Remove enableFilterEmptyStringAttributesDOM
rickhanlonii Dec 13, 2024
ad3968d
Merge branch 'main' into rh/rm-ff-filter-strings
rickhanlonii Dec 13, 2024
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
144 changes: 69 additions & 75 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,7 @@ import {validateProperties as validateInputProperties} from '../shared/ReactDOMN
import {validateProperties as validateUnknownProperties} from '../shared/ReactDOMUnknownPropertyHook';
import sanitizeURL from '../shared/sanitizeURL';

import {
enableTrustedTypesIntegration,
enableFilterEmptyStringAttributesDOM,
} from 'shared/ReactFeatureFlags';
import {enableTrustedTypesIntegration} from 'shared/ReactFeatureFlags';
import {
mediaEventTypes,
listenToNonDelegatedEvent,
Expand Down Expand Up @@ -400,35 +397,33 @@ function setProp(
// fallthrough
case 'src':
case 'href': {
if (enableFilterEmptyStringAttributesDOM) {
if (
value === '' &&
// <a href=""> is fine for "reload" links.
!(tag === 'a' && key === 'href')
) {
if (__DEV__) {
if (key === 'src') {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'This may cause the browser to download the whole page again over the network. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
key,
key,
);
} else {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
key,
key,
);
}
if (
value === '' &&
// <a href=""> is fine for "reload" links.
!(tag === 'a' && key === 'href')
) {
if (__DEV__) {
if (key === 'src') {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'This may cause the browser to download the whole page again over the network. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
key,
key,
);
} else {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
key,
key,
);
}
domElement.removeAttribute(key);
break;
}
domElement.removeAttribute(key);
break;
}
if (
value == null ||
Expand Down Expand Up @@ -2489,53 +2484,52 @@ function diffHydratedGenericElement(
// fallthrough
case 'src':
case 'href':
if (enableFilterEmptyStringAttributesDOM) {
if (
value === '' &&
// <a href=""> is fine for "reload" links.
!(tag === 'a' && propKey === 'href') &&
!(tag === 'object' && propKey === 'data')
) {
if (__DEV__) {
if (propKey === 'src') {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'This may cause the browser to download the whole page again over the network. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
propKey,
propKey,
);
} else {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
propKey,
propKey,
);
}
if (
value === '' &&
// <a href=""> is fine for "reload" links.
!(tag === 'a' && propKey === 'href') &&
!(tag === 'object' && propKey === 'data')
) {
if (__DEV__) {
if (propKey === 'src') {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'This may cause the browser to download the whole page again over the network. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
propKey,
propKey,
);
} else {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
propKey,
propKey,
);
}
Copy link
Collaborator

@sebmarkbage sebmarkbage Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should probably skip hydrateSanitizedAttribute below if we already warned above.

I.e. call continue in the branch above.

Otherwise we end up both logging the error and with a hydration error, because the server will have omitted this attribute.

The hydration error is also not correct because it's not what the client would've done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean only in the DEV branch above? That seems weird to differ in behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what you're saying is that this would be how to fix the bug you found, so i'll merge with the existing behavior and we can fix the bug in a follow up

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well the DEV branch is defensive. This whole function is inside a DEV branch so it's not divergent.

Copy link
Member Author

Choose a reason for hiding this comment

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

#31783

hydrateSanitizedAttribute(
domElement,
propKey,
propKey,
null,
extraAttributes,
serverDifferences,
);
continue;
}
hydrateSanitizedAttribute(
domElement,
propKey,
propKey,
null,
extraAttributes,
serverDifferences,
);
continue;
} else {
hydrateSanitizedAttribute(
domElement,
propKey,
propKey,
value,
extraAttributes,
serverDifferences,
);
continue;
}
hydrateSanitizedAttribute(
domElement,
propKey,
propKey,
value,
extraAttributes,
serverDifferences,
);
continue;
case 'action':
case 'formAction': {
const serverValue = domElement.getAttribute(propKey);
Expand Down
75 changes: 32 additions & 43 deletions packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@ import {

import {Children} from 'react';

import {
enableFilterEmptyStringAttributesDOM,
enableFizzExternalRuntime,
} from 'shared/ReactFeatureFlags';
import {enableFizzExternalRuntime} from 'shared/ReactFeatureFlags';

import type {
Destination,
Expand Down Expand Up @@ -1210,30 +1207,28 @@ function pushAttribute(
}
case 'src':
case 'href': {
if (enableFilterEmptyStringAttributesDOM) {
if (value === '') {
if (__DEV__) {
if (name === 'src') {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'This may cause the browser to download the whole page again over the network. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
name,
name,
);
} else {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
name,
name,
);
}
if (value === '') {
if (__DEV__) {
if (name === 'src') {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'This may cause the browser to download the whole page again over the network. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
name,
name,
);
} else {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
name,
name,
);
}
return;
}
return;
}
}
// Fall through to the last case which shouldn't remove empty strings.
Expand Down Expand Up @@ -1633,19 +1628,17 @@ function pushStartObject(
checkAttributeStringCoercion(propValue, 'data');
}
const sanitizedValue = sanitizeURL('' + propValue);
if (enableFilterEmptyStringAttributesDOM) {
if (sanitizedValue === '') {
if (__DEV__) {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
propKey,
propKey,
);
}
break;
if (sanitizedValue === '') {
if (__DEV__) {
console.error(
'An empty string ("") was passed to the %s attribute. ' +
'To fix this, either do not render the element at all ' +
'or pass null to %s instead of an empty string.',
propKey,
propKey,
);
}
break;
}
target.push(
attributeSeparator,
Expand Down Expand Up @@ -3615,11 +3608,7 @@ export function pushStartInstance(
// Fast track very common tags
break;
case 'a':
if (enableFilterEmptyStringAttributesDOM) {
return pushStartAnchor(target, props);
} else {
break;
}
return pushStartAnchor(target, props);
case 'g':
case 'p':
case 'li':
Expand Down
Loading
Loading