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

Match Preact behavior for boolean props on custom elements #24541

Merged
merged 4 commits into from
May 20, 2022

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented May 11, 2022

Summary

Closes #9230 gated behind enableCustomElementPropertySupport i.e. react-dom@experimental

Implements #9230 (comment) with the exception of

 val = true;
 <x-foo bool={val}></x-foo>
-// output: <x-foo bool="true"></x-foo>
+// output: <x-foo bool=""></x-foo>

How did you test this change?

  • CI
  • Run attribute fixtures (should not be affected since this change is gated behind enableCustomElementPropertySupport
  • https://custom-elements-everywhere.com/ with a build from this PR in libraries/react-experimental still passes all their tests

@@ -89,7 +89,7 @@ module.exports = function(initModules) {
console.log(
`We expected ${count} warning(s), but saw ${filteredWarnings.length} warning(s).`,
);
if (filteredWarnings.count > 0) {
if (filteredWarnings.length > 0) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Drive-by fix that was required to get hydration mismatch warnings during development. This adds some noise to this test since there are a bunch of tests that expect more warnings than there actually are.

I started working on them by switching to toErrorDev instead and it's a lot of work to fully switch. Though it revealed some odd error messages and that you also hit #22797 in your tests. So definitely worth pursuing IMO.

eps1lon added 2 commits May 11, 2022 20:48
Following facebook#9230 (comment) except that `foo={true}` renders an empty string.
See facebook#9230 (comment) for rationale.
@eps1lon eps1lon force-pushed the web-components-boolean-props branch from 5fb84e7 to 1c4f102 Compare May 11, 2022 18:49
@sizebot
Copy link

sizebot commented May 11, 2022

Comparing: 8197c73...ad479ab

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 = 131.58 kB 131.58 kB = 42.15 kB 42.15 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.02% 136.82 kB 136.85 kB +0.03% 43.68 kB 43.69 kB
facebook-www/ReactDOM-prod.classic.js = 440.68 kB 440.68 kB = 80.33 kB 80.33 kB
facebook-www/ReactDOM-prod.modern.js +0.02% 425.89 kB 425.97 kB +0.01% 78.15 kB 78.16 kB
facebook-www/ReactDOMForked-prod.classic.js = 440.68 kB 440.68 kB = 80.33 kB 80.33 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against ad479ab

@gaearon
Copy link
Collaborator

gaearon commented May 11, 2022

We should get @josepharhar sign off on this

@eps1lon eps1lon marked this pull request as ready for review May 11, 2022 22:07
@josepharhar
Copy link
Contributor

Thanks for looking into this! I believe this is a good idea, and I can't find any reasons/history against going for this behavior.

Rendering as <x-foo attr=""> instead of <x-foo attr="true"> makes sense to me based on the discussion in the issue, I don't know why Preact does this. Based on my testing, Preact actually renders it as <x-foo attr=""> when server rendering, but as <x-foo attr="true"> when client rendering...

@gaearon gaearon merged commit 82c64e1 into facebook:main May 20, 2022
@eps1lon eps1lon deleted the web-components-boolean-props branch May 20, 2022 18:14
@michaelwarren1106
Copy link

is there a target release version this PR will land in? I'm curious about what to tell devs that are looking for this behavior

@eps1lon
Copy link
Collaborator Author

eps1lon commented May 23, 2022

is there a target release version this PR will land in? I'm curious about what to tell devs that are looking for this behavior

It's out in react-dom@0.0.0-experimental-82c64e1a4-20220520: https://codesandbox.io/s/boring-ardinghelli-h6wpc1?file=/src/index.js

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.

Boolean attributes on Web Components
6 participants