Restore old behavior for empty `href` props on anchor tags #28124

eps1lon · 2024-01-27T09:38:50Z

Summary

Treat <a href="" /> the same with and without enableFilterEmptyStringAttributesDOM

in #18513 we started to warn and ignore for empty href and src props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since <a href=""></a> will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances.

The implementation for Fizz is in the spirit of #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.

How did you test this change?

Added tests to server integration suite
checked attribute-behavior fixture

e.g. `<a href="" />`

react-sizebot · 2024-01-27T09:42:41Z

Comparing: 9aef5d2...951d18e

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	176.59 kB	176.59 kB	=	55.01 kB	55.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.01%	178.69 kB	178.71 kB	+0.02%	55.64 kB	55.65 kB
facebook-www/ReactDOM-prod.classic.js	=	573.52 kB	573.56 kB	=	100.99 kB	101.00 kB
facebook-www/ReactDOM-prod.modern.js	=	557.29 kB	557.32 kB	+0.01%	98.07 kB	98.08 kB
test_utils/ReactAllWarnings.js	Deleted	67.55 kB	0.00 kB	Deleted	16.54 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
facebook-www/ReactDOMServer-prod.modern.js	+0.75%	198.82 kB	200.32 kB	+0.38%	35.97 kB	36.11 kB
facebook-www/ReactDOMServer-prod.classic.js	+0.75%	199.99 kB	201.48 kB	+0.39%	36.25 kB	36.39 kB
facebook-www/ReactDOMServerStreaming-prod.modern.js	+0.74%	202.63 kB	204.12 kB	+0.39%	37.21 kB	37.35 kB
oss-experimental/react-dom/cjs/react-dom-server.bun.production.js	+0.44%	327.41 kB	328.86 kB	+0.20%	70.87 kB	71.01 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.js	+0.44%	329.28 kB	330.73 kB	+0.18%	71.34 kB	71.47 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.js	+0.43%	335.45 kB	336.90 kB	+0.19%	73.22 kB	73.36 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.js	+0.43%	337.94 kB	339.39 kB	+0.16%	72.45 kB	72.57 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.js	+0.43%	340.29 kB	341.74 kB	+0.18%	72.89 kB	73.02 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.production.js	+0.42%	342.46 kB	343.90 kB	+0.18%	73.89 kB	74.02 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.production.min.js	+0.41%	82.94 kB	83.29 kB	+0.36%	25.52 kB	25.61 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.production.min.js	+0.41%	82.87 kB	83.20 kB	+0.37%	25.13 kB	25.22 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.production.min.js	+0.40%	87.32 kB	87.67 kB	+0.29%	26.64 kB	26.71 kB
oss-experimental/react-dom/umd/react-dom-server.browser.production.min.js	+0.40%	87.38 kB	87.73 kB	+0.40%	26.96 kB	27.07 kB
oss-experimental/react-dom/cjs/react-dom-server.bun.production.min.js	+0.39%	85.74 kB	86.08 kB	+0.39%	25.92 kB	26.02 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.production.min.js	+0.38%	88.77 kB	89.11 kB	+0.35%	27.09 kB	27.18 kB
oss-experimental/react-dom/cjs/react-dom-server.node.production.min.js	+0.38%	90.92 kB	91.26 kB	+0.37%	27.55 kB	27.66 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.production.min.js	+0.37%	92.69 kB	93.04 kB	+0.26%	28.39 kB	28.46 kB
facebook-www/ReactDOMServerStreaming-dev.modern.js	+0.35%	465.26 kB	466.88 kB	+0.14%	94.92 kB	95.05 kB
facebook-www/ReactDOMServer-dev.modern.js	+0.34%	472.61 kB	474.23 kB	+0.14%	96.57 kB	96.71 kB
oss-experimental/react-dom/umd/react-dom-server-legacy.browser.development.js	+0.34%	452.08 kB	453.62 kB	+0.14%	97.83 kB	97.96 kB
oss-experimental/react-dom/cjs/react-dom-server.bun.development.js	+0.34%	425.61 kB	427.06 kB	+0.14%	94.86 kB	95.00 kB
facebook-www/ReactDOMServer-dev.classic.js	+0.34%	481.05 kB	482.67 kB	+0.14%	98.26 kB	98.40 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.browser.development.js	+0.33%	432.02 kB	433.47 kB	+0.13%	96.84 kB	96.97 kB
oss-experimental/react-dom/umd/react-dom-server.browser.development.js	+0.33%	461.22 kB	462.76 kB	+0.13%	98.98 kB	99.12 kB
oss-experimental/react-dom/cjs/react-dom-server-legacy.node.development.js	+0.33%	433.88 kB	435.32 kB	+0.13%	97.30 kB	97.43 kB
oss-experimental/react-dom/cjs/react-dom-server.node.development.js	+0.33%	439.19 kB	440.64 kB	+0.14%	97.15 kB	97.29 kB
oss-experimental/react-dom/cjs/react-dom-server.browser.development.js	+0.33%	440.76 kB	442.21 kB	+0.14%	97.93 kB	98.06 kB
oss-experimental/react-dom/cjs/react-dom-server.edge.development.js	+0.33%	441.35 kB	442.79 kB	+0.14%	98.06 kB	98.20 kB
test_utils/ReactAllWarnings.js	Deleted	67.55 kB	0.00 kB	Deleted	16.54 kB	0.00 kB

Generated by 🚫 dangerJS against 951d18e

gnoff · 2024-01-30T21:33:02Z

packages/react-dom-bindings/src/client/ReactDOMComponent.js

+        if (
+          value === '' &&
+          // <a href=""> is fine for "reload" links.
+          tag !== 'a'


why are we distinguishing href from src in the hydration branch but not here?

Forgot. But (tag !== 'a' || propKey !== 'href') is not very nice to read so I went with !(tag === 'a' && key === 'href') to match the comment better.

gnoff · 2024-01-30T21:36:46Z

packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js

+    case 'a':
+      if (enableFilterEmptyStringAttributesDOM) {
+        return pushStartAnchor(target, props);
+      } else {
+        break;
+      }


not sure the ordering was picked with scientific rigor but since a is pretty common maybe put it back where it was and just add the extra break before it?

If I put it back up I have to make an additional check that we're dealing with an a tag so that e.g. div wouldn't fall into pushStartAnchor.

As far as I can tell, I can only put it before the common group or after but not in between without having an additional check. But then maybe I'm missing some neat tricks with switch statements that I haven't thought of.

just add the extra break before it?

That was the important bit I missed.

Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in #18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off. DiffTrain build for [f3ce87a](f3ce87a)

## Summary Stacked on [#28124](#28124) ([Diff against #28124](eps1lon:fix/anchor-href-empty...eps1lon:feat/enableFilterEmptyStringAttributesDOM-canary)) Enables `enableFilterEmptyStringAttributesDOM` everywhere. I don't think this needs to be enabled behind `__VARIANT__` for RN since this flag is only used in DOM. ## How did you test this change? - CI

Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in #18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.

## Summary Stacked on [#28124](#28124) ([Diff against #28124](eps1lon:fix/anchor-href-empty...eps1lon:feat/enableFilterEmptyStringAttributesDOM-canary)) Enables `enableFilterEmptyStringAttributesDOM` everywhere. I don't think this needs to be enabled behind `__VARIANT__` for RN since this flag is only used in DOM. ## How did you test this change? - CI

…28124) Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in facebook#18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of facebook#21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off.

## Summary Stacked on [facebook#28124](facebook#28124) ([Diff against facebook#28124](facebook/react@eps1lon:fix/anchor-href-empty...eps1lon:feat/enableFilterEmptyStringAttributesDOM-canary)) Enables `enableFilterEmptyStringAttributesDOM` everywhere. I don't think this needs to be enabled behind `__VARIANT__` for RN since this flag is only used in DOM. ## How did you test this change? - CI

Treat `<a href="" />` the same with and without `enableFilterEmptyStringAttributesDOM` in #18513 we started to warn and ignore for empty `href` and `src` props since it usually hinted at a mistake. However, for anchor tags there's a valid use case since `<a href=""></a>` will by spec render a link to the current page. It could be used to reload the page without having to rely on browser affordances. The implementation for Fizz is in the spirit of #21153. I gated the fork behind the flag so that the fork is DCE'd when the flag is off. DiffTrain build for commit f3ce87a.

Restore old behavior for empty href props on anchor tags

1a5d710

e.g. `<a href="" />`

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 27, 2024

eps1lon marked this pull request as ready for review January 27, 2024 09:52

eps1lon requested a review from sebmarkbage January 27, 2024 09:52

eps1lon mentioned this pull request Jan 27, 2024

Enable enableFilterEmptyStringAttributesDOM everywhere #28125

Merged

gnoff reviewed Jan 30, 2024

View reviewed changes

Sebastian Silbermann added 2 commits January 30, 2024 22:56

code matches comments

8c99435

Now I get it

951d18e

gnoff approved these changes Jan 30, 2024

View reviewed changes

eps1lon merged commit f3ce87a into facebook:main Jan 30, 2024
36 checks passed

eps1lon deleted the fix/anchor-href-empty branch January 30, 2024 23:43

gnoff mentioned this pull request Feb 1, 2024

Update React from 60a927d04 to 2bc7d336a vercel/next.js#61522

Merged

ztanner mentioned this pull request Mar 19, 2024

(backport) Update React from 60a927d04 to 14898b6a9 vercel/next.js#63442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore old behavior for empty `href` props on anchor tags #28124

Restore old behavior for empty `href` props on anchor tags #28124

eps1lon commented Jan 27, 2024

react-sizebot commented Jan 27, 2024 •

edited

Loading

gnoff Jan 30, 2024

eps1lon Jan 30, 2024

gnoff Jan 30, 2024

eps1lon Jan 30, 2024

eps1lon Jan 30, 2024

Restore old behavior for empty href props on anchor tags #28124

Restore old behavior for empty href props on anchor tags #28124

Conversation

eps1lon commented Jan 27, 2024

Summary

How did you test this change?

react-sizebot commented Jan 27, 2024 • edited Loading

Critical size changes

Significant size changes

gnoff Jan 30, 2024

Choose a reason for hiding this comment

eps1lon Jan 30, 2024

Choose a reason for hiding this comment

gnoff Jan 30, 2024

Choose a reason for hiding this comment

eps1lon Jan 30, 2024

Choose a reason for hiding this comment

eps1lon Jan 30, 2024

Choose a reason for hiding this comment

Restore old behavior for empty `href` props on anchor tags #28124

Restore old behavior for empty `href` props on anchor tags #28124

react-sizebot commented Jan 27, 2024 •

edited

Loading