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

[Fizz] Implement all the DOM attributes and special cases #21153

Merged
merged 9 commits into from
Apr 1, 2021

Conversation

sebmarkbage
Copy link
Collaborator

This implements all the DOM bindings parts as far as I know (except the reactroot attribute). I can't run it against our test suite yet. But I carefully ported everything I could.

There's only one breakage I decided to do. We don't force lower case HTML tags before comparing them. This means that special cases like <select />, <textarea /> etc. don't work if you use a different case. We warned about this before and warn about it after, so shouldn't matter.

I did a very different structure from what we've done in the DOM bindings before though.

A problem with how we used to do it is that the logic is spread out into a lot of different concepts. This causes a lot of extra checks for each element because we check those things over and over. It also leads to unnecessary checks. A lot of the helpers check things like == null over and over or they overcheck because in that context it can never happen.

Instead of a bunch of isSomething(...) helpers. I structured it fully based on a switch over each kind of value once and from there the code flow determines what to do next.

The structure is something like this:

  • type
    • propertyName
      • propertyInfo
        • value

First we switch on the type, then we loop over each property, we switch over the property name to find special cases, then we switch over the propertyInfo and finally the value type and that determines what we do.

So the goal is that we only check each of these once along that path.

All special cases are encoded along with that specific element type. This ends up needing to fork the loop. So there's a lot of duplication there. This is controversial because if you make a change to these things you have to replicate them. However, I've noticed subtle little issues because we don't consider how each special case applies to each tag. So this is actually a benefit. It's also better for perf.

Large switches might not be better than Map of Functions depending on VM but that's something we can automate if needed if it's expressed as a simple switch.

@sebmarkbage sebmarkbage requested a review from gaearon March 31, 2021 22:33
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 31, 2021
@sizebot
Copy link

sizebot commented Mar 31, 2021

Comparing: f8ef4ff...dbc8fb9

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 = 122.65 kB 122.65 kB = 39.35 kB 39.35 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 129.21 kB 129.21 kB = 41.43 kB 41.43 kB
facebook-www/ReactDOM-prod.classic.js = 405.89 kB 405.89 kB = 75.39 kB 75.39 kB
facebook-www/ReactDOM-prod.modern.js = 394.15 kB 394.15 kB = 73.53 kB 73.53 kB
facebook-www/ReactDOMForked-prod.classic.js = 405.89 kB 405.89 kB = 75.39 kB 75.39 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +125.35% 62.12 kB 139.99 kB +118.31% 14.81 kB 32.33 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +125.23% 59.05 kB 132.99 kB +119.06% 14.55 kB 31.87 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +125.12% 59.10 kB 133.05 kB +118.62% 14.63 kB 31.98 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.production.min.js +73.90% 13.92 kB 24.21 kB +68.66% 4.85 kB 8.18 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.production.min.js +72.70% 14.11 kB 24.37 kB +66.90% 4.95 kB 8.26 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.production.min.js +72.09% 14.25 kB 24.52 kB +67.89% 4.89 kB 8.20 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.development.js +125.35% 62.12 kB 139.99 kB +118.31% 14.81 kB 32.33 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.development.js +125.23% 59.05 kB 132.99 kB +119.06% 14.55 kB 31.87 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.development.js +125.12% 59.10 kB 133.05 kB +118.62% 14.63 kB 31.98 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.browser.production.min.js +73.90% 13.92 kB 24.21 kB +68.66% 4.85 kB 8.18 kB
oss-experimental/react-dom/umd/react-dom-unstable-fizz.browser.production.min.js +72.70% 14.11 kB 24.37 kB +66.90% 4.95 kB 8.26 kB
oss-experimental/react-dom/cjs/react-dom-unstable-fizz.node.production.min.js +72.09% 14.25 kB 24.52 kB +67.89% 4.89 kB 8.20 kB
oss-experimental/react-server/cjs/react-server.production.min.js +0.89% 9.57 kB 9.65 kB +0.69% 3.34 kB 3.36 kB
oss-stable/react-server/cjs/react-server.production.min.js +0.89% 9.57 kB 9.65 kB +0.69% 3.34 kB 3.36 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +0.69% 2.62 kB 2.63 kB +0.48% 1.04 kB 1.05 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.production.min.js +0.69% 2.62 kB 2.63 kB +0.48% 1.04 kB 1.05 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-server.development.js +0.50% 5.44 kB 5.47 kB +0.51% 1.56 kB 1.56 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-server.development.js +0.50% 5.44 kB 5.47 kB +0.51% 1.56 kB 1.56 kB
oss-experimental/react-server/cjs/react-server.development.js +0.42% 36.67 kB 36.82 kB +0.21% 9.60 kB 9.62 kB
oss-stable/react-server/cjs/react-server.development.js +0.42% 36.67 kB 36.82 kB +0.21% 9.60 kB 9.62 kB

Generated by 🚫 dangerJS against dbc8fb9

We read the currently selected value from the FormatContext.
We don't change to lower case at runtime anymore but keep the warning.
This is because if you do the equivalent on the client using innerHTML,
this is the effect you'd get.
@gaearon gaearon mentioned this pull request Apr 7, 2021
63 tasks
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 11, 2021
…1153)

* Implement DOM format config structure

* Styles

* Input warnings

* Textarea special cases

* Select special cases

* Option special cases

We read the currently selected value from the FormatContext.

* Warning for non-lower case HTML

We don't change to lower case at runtime anymore but keep the warning.

* Pre tags innerHTML needs to be prefixed

This is because if you do the equivalent on the client using innerHTML,
this is the effect you'd get.

* Extract errors
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 13, 2021
…1153)

* Implement DOM format config structure

* Styles

* Input warnings

* Textarea special cases

* Select special cases

* Option special cases

We read the currently selected value from the FormatContext.

* Warning for non-lower case HTML

We don't change to lower case at runtime anymore but keep the warning.

* Pre tags innerHTML needs to be prefixed

This is because if you do the equivalent on the client using innerHTML,
this is the effect you'd get.

* Extract errors
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 16, 2021
…1153)

* Implement DOM format config structure

* Styles

* Input warnings

* Textarea special cases

* Select special cases

* Option special cases

We read the currently selected value from the FormatContext.

* Warning for non-lower case HTML

We don't change to lower case at runtime anymore but keep the warning.

* Pre tags innerHTML needs to be prefixed

This is because if you do the equivalent on the client using innerHTML,
this is the effect you'd get.

* Extract errors
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 16, 2021
…1153)

* Implement DOM format config structure

* Styles

* Input warnings

* Textarea special cases

* Select special cases

* Option special cases

We read the currently selected value from the FormatContext.

* Warning for non-lower case HTML

We don't change to lower case at runtime anymore but keep the warning.

* Pre tags innerHTML needs to be prefixed

This is because if you do the equivalent on the client using innerHTML,
this is the effect you'd get.

* Extract errors
acdlite pushed a commit to acdlite/react that referenced this pull request Apr 19, 2021
…1153)

* Implement DOM format config structure

* Styles

* Input warnings

* Textarea special cases

* Select special cases

* Option special cases

We read the currently selected value from the FormatContext.

* Warning for non-lower case HTML

We don't change to lower case at runtime anymore but keep the warning.

* Pre tags innerHTML needs to be prefixed

This is because if you do the equivalent on the client using innerHTML,
this is the effect you'd get.

* Extract errors
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 20, 2021
Summary:
This sync includes the following changes:
- **[f7cdc8936](facebook/react@f7cdc8936 )**: Also turn off enableSyncDefaultUpdates in RN test renderer ([#21293](facebook/react#21293)) //<Ricky>//
- **[4c9eb2af1](facebook/react@4c9eb2af1 )**: Add dynamic flags to React Native ([#21291](facebook/react#21291)) //<Ricky>//
- **[9eddfbf5a](facebook/react@9eddfbf5a )**: [Fizz] Two More Fixes ([#21288](facebook/react#21288)) //<Sebastian Markbåge>//
- **[11b07597e](facebook/react@11b07597e )**: Fix classes ([#21283](facebook/react#21283)) //<Sebastian Markbåge>//
- **[96d00b9bb](facebook/react@96d00b9bb )**: [Fizz] Random Fixes ([#21277](facebook/react#21277)) //<Sebastian Markbåge>//
- **[81ef53953](facebook/react@81ef53953 )**: Always insert a dummy node with an ID into fallbacks ([#21272](facebook/react#21272)) //<Sebastian Markbåge>//
- **[a4a940d7a](facebook/react@a4a940d7a )**: [Fizz] Add unsupported Portal/Scope components ([#21261](facebook/react#21261)) //<Sebastian Markbåge>//
- **[f4d7a0f1e](facebook/react@f4d7a0f1e )**: Implement useOpaqueIdentifier ([#21260](facebook/react#21260)) //<Sebastian Markbåge>//
- **[dde875dfb](facebook/react@dde875dfb )**: [Fizz] Implement Hooks ([#21257](facebook/react#21257)) //<Sebastian Markbåge>//
- **[a597c2f5d](facebook/react@a597c2f5d )**: [Fizz] Fix reentrancy bug ([#21270](facebook/react#21270)) //<Sebastian Markbåge>//
- **[15e779d92](facebook/react@15e779d92 )**: Reconciler should inject its own version into DevTools hook ([#21268](facebook/react#21268)) //<Brian Vaughn>//
- **[4f76a28c9](facebook/react@4f76a28c9 )**: [Fizz] Implement New Context ([#21255](facebook/react#21255)) //<Sebastian Markbåge>//
- **[82ef450e0](facebook/react@82ef450e0 )**: remove obsolete SharedArrayBuffer ESLint config ([#21259](facebook/react#21259)) //<Henry Q. Dineen>//
- **[dbadfa2c3](facebook/react@dbadfa2c3 )**: [Fizz] Classes Follow Up ([#21253](facebook/react#21253)) //<Sebastian Markbåge>//
- **[686b635b7](facebook/react@686b635b7 )**: Prevent reading canonical property of null ([#21242](facebook/react#21242)) //<Joshua Gross>//
- **[bb88ce95a](facebook/react@bb88ce95a )**: Bugfix: Don't rely on `finishedLanes` for passive effects ([#21233](facebook/react#21233)) //<Andrew Clark>//
- **[343710c92](facebook/react@343710c92 )**: [Fizz] Fragments and Iterable support ([#21228](facebook/react#21228)) //<Sebastian Markbåge>//
- **[933880b45](facebook/react@933880b45 )**: Make time-slicing opt-in ([#21072](facebook/react#21072)) //<Ricky>//
- **[b0407b55f](facebook/react@b0407b55f )**: Support more empty types ([#21225](facebook/react#21225)) //<Sebastian Markbåge>//
- **[39713716a](facebook/react@39713716a )**: Merge isObject branches ([#21226](facebook/react#21226)) //<Sebastian Markbåge>//
- **[8a4a59c72](facebook/react@8a4a59c72 )**: Remove textarea special case from child fiber ([#21222](facebook/react#21222)) //<Sebastian Markbåge>//
- **[dc108b0f5](facebook/react@dc108b0f5 )**: Track which fibers scheduled the current render work ([#15658](facebook/react#15658)) //<Brian Vaughn>//
- **[6ea749170](facebook/react@6ea749170 )**: Fix typo in comment ([#21214](facebook/react#21214)) //<inokawa>//
- **[b38ac13f9](facebook/react@b38ac13f9 )**: DevTools: Add post-commit hook ([#21183](facebook/react#21183)) //<Brian Vaughn>//
- **[b943aeba8](facebook/react@b943aeba8 )**: Fix: Passive effect updates are never sync ([#21215](facebook/react#21215)) //<Andrew Clark>//
- **[d389c54d1](facebook/react@d389c54d1 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21211](facebook/react#21211)) //<Brian Vaughn>//
- **[c486dc1a4](facebook/react@c486dc1a4 )**: Remove unnecessary processUpdateQueue ([#21199](facebook/react#21199)) //<Sebastian Markbåge>//
- **[cf45a623a](facebook/react@cf45a623a )**: [Fizz] Implement Classes ([#21200](facebook/react#21200)) //<Sebastian Markbåge>//
- **[75c616554](facebook/react@75c616554 )**: Include actual type of `Profiler#id` on type mismatch ([#20306](facebook/react#20306)) //<Sebastian Silbermann>//
- **[1214b302e](facebook/react@1214b302e )**: test: Fix "couldn't locate all inline snapshots" ([#21205](facebook/react#21205)) //<Sebastian Silbermann>//
- **[1a02d2792](facebook/react@1a02d2792 )**: style: delete unused isHost check ([#21203](facebook/react#21203)) //<wangao>//
- **[782f689ca](facebook/react@782f689ca )**: Don't double invoke getDerivedStateFromProps for module pattern ([#21193](facebook/react#21193)) //<Sebastian Markbåge>//
- **[e90c76a65](facebook/react@e90c76a65 )**: Revert "Offscreen: Use JS stack to track hidden/unhidden subtree state" ([#21194](facebook/react#21194)) //<Brian Vaughn>//
- **[1f8583de8](facebook/react@1f8583de8 )**: Offscreen: Use JS stack to track hidden/unhidden subtree state ([#21192](facebook/react#21192)) //<Brian Vaughn>//
- **[ad6e6ec7b](facebook/react@ad6e6ec7b )**: [Fizz] Prepare Recursive Loop for More Types ([#21186](facebook/react#21186)) //<Sebastian Markbåge>//
- **[172e89b4b](facebook/react@172e89b4b )**: Reland Remove redundant initial of isArray ([#21188](facebook/react#21188)) //<Sebastian Markbåge>//
- **[7c1ba2b57](facebook/react@7c1ba2b57 )**: Proposed new Suspense layout effect semantics ([#21079](facebook/react#21079)) //<Brian Vaughn>//
- **[316aa3686](facebook/react@316aa3686 )**: [Scheduler] Fix de-opt caused by out-of-bounds access ([#21147](facebook/react#21147)) //<Andrey Marchenko>//
- **[b4f119cdf](facebook/react@b4f119cdf )**: Revert "Remove redundant initial of isArray ([#21163](facebook/react#21163))" //<Sebastian Markbage>//
- **[c03197063](facebook/react@c03197063 )**: Revert "apply prettier ([#21165](facebook/react#21165))" //<Sebastian Markbage>//
- **[94fd1214d](facebook/react@94fd1214d )**: apply prettier ([#21165](facebook/react#21165)) //<Behnam Mohammadi>//
- **[b130a0f5c](facebook/react@b130a0f5c )**: Remove redundant initial of isArray ([#21163](facebook/react#21163)) //<Behnam Mohammadi>//
- **[2c9fef32d](facebook/react@2c9fef32d )**: Remove redundant initial of hasOwnProperty ([#21134](facebook/react#21134)) //<Behnam Mohammadi>//
- **[1cf9978d8](facebook/react@1cf9978d8 )**: Don't pass internals to callbacks ([#21161](facebook/react#21161)) //<Sebastian Markbåge>//
- **[b9e4c10e9](facebook/react@b9e4c10e9 )**: [Fizz] Implement all the DOM attributes and special cases ([#21153](facebook/react#21153)) //<Sebastian Markbåge>//
- **[f8ef4ff57](facebook/react@f8ef4ff57 )**: Flush discrete passive effects before paint ([#21150](facebook/react#21150)) //<Andrew Clark>//
- **[b48b38af6](facebook/react@b48b38af6 )**: Support nesting of startTransition and flushSync (alt) ([#21149](facebook/react#21149)) //<Sebastian Markbåge>//

Changelog:
[General][Changed] - React Native sync for revisions c9aab1c...f7cdc89

jest_e2e[run_all_tests]

Reviewed By: rickhanlonii

Differential Revision: D27740113

fbshipit-source-id: 6e27204d78e3e16ed205170006cb97c0d6bfa957
koto pushed a commit to koto/react that referenced this pull request Jun 15, 2021
…1153)

* Implement DOM format config structure

* Styles

* Input warnings

* Textarea special cases

* Select special cases

* Option special cases

We read the currently selected value from the FormatContext.

* Warning for non-lower case HTML

We don't change to lower case at runtime anymore but keep the warning.

* Pre tags innerHTML needs to be prefixed

This is because if you do the equivalent on the client using innerHTML,
this is the effect you'd get.

* Extract errors
eps1lon added a commit that referenced this pull request Jan 30, 2024
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.
github-actions bot pushed a commit that referenced this pull request Jan 30, 2024
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)
gaearon pushed a commit that referenced this pull request Feb 3, 2024
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.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants