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

WIP: Custom Attribute and Preferring Attribute Names over Properties Spike #10229

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@nhunzaker
Collaborator

nhunzaker commented Jul 20, 2017

Do not merge this PR.

This is a work in progress PR to investigate the outcome of aggressively removing entries in the attributes lists and moving over to preferring attribute names over properties. My goals are:

  • What startup improvements do we achieve from avoiding numeration over many properties
  • What edge cases might be exposed to library authors
  • What is the improvement in authoring experience
  • What is the easiest migration path, and what would it look like to support both property and attribute names (className and class)
  • What code can we eliminate beyond the whitelists that is built around them
  • Prevent event names from showing up on server-rendering

I'm probably missing a few more, but I thought it might be neat to track this progression on Github.


Performance improvements (still TBD)

There are two factors I see: startup time and attribute generation time.

This PR avoids enumerating over a big list of properties at startup, but that process is pretty fast. More time is actually spent enumerating over the event list in SimpleEventPlugin. I'm seeing DOM property injection take from 1.5 to 2 milliseconds. Cutting 2ms isn't huge, but I'll try to get some better numbers.

  • is the 1.5-2 ms property injection number accurate?
  • Without the custom attribute regexes and extra branches, is attribute assignment faster?

Build size differences

Even with manually listing out the properties that we need special behavior for, there's still some savings:

┌──────────────────────────────────────────────────┬───────────┬──────────────┬───────┬───────────┬──────────────┬───────┐
│ Bundle                                           │ Prev Size │ Current Size │ Diff  │ Prev Gzip │ Current Gzip │ Diff  │
├──────────────────────────────────────────────────┼───────────┼──────────────┼───────┼───────────┼──────────────┼───────┤
│ react-dom.production.min.js (NODE_PROD)          │ 116.05 KB │ 110.97 KB    │ -5 %  │ 36.9 KB   │ 34.82 KB     │ -6 %  │
├──────────────────────────────────────────────────┼───────────┼──────────────┼───────┼───────────┼──────────────┼───────┤
│ react-dom-server.production.min.js (NODE_PROD)   │ 20.72 KB  │ 14.65 KB     │ -30 % │ 7.84 KB   │ 5.6 KB       │ -29 % │
└──────────────────────────────────────────────────┴───────────┴──────────────┴───────┴───────────┴──────────────┴───────┘

ReactDOM his -6% gzipped. Pretty healthy cut. The server build drops by 30% in size, which isn't all that grand but fun to say at least 😄.

Improvement to authoring

TBD

Edge cases

TBD

Migration path for libraries

TBD

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 20, 2017

Collaborator

An interesting side effect of this is you no longer get helpful errors about attributes being spelt wrong yeah? I've always quite enjoyed that react does that especially for weirdness like aria-labelledby It would be nice to maintain some of those helpful warnings behind a __DEV__ only build perhaps

Collaborator

jquense commented Jul 20, 2017

An interesting side effect of this is you no longer get helpful errors about attributes being spelt wrong yeah? I've always quite enjoyed that react does that especially for weirdness like aria-labelledby It would be nice to maintain some of those helpful warnings behind a __DEV__ only build perhaps

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 20, 2017

Collaborator

@jquense WIP! I think it would be great to maintain a manifest of all HTML attributes that we require inside __DEV__ for the unknown property hooks.

I've also pushed a new update:

  • I removed the HTML, SVG, and ARIA configs, consolidating all of their rules into the DOM Property config. This actually ends up saving some space post GZIP and avoids needing to enumerate over property configs at boot.
  • I've removed boolean checks for features like isNumeric and isBoolean in favor of object maps for properties that need those settings. This is another experiment to evaluate the performance trade off.

All in all, even if we manually specify all configuration options for every single attribute, the GZIP size ends up being rough the same (or a bit lower) than enumerating over individual lists.

Collaborator

nhunzaker commented Jul 20, 2017

@jquense WIP! I think it would be great to maintain a manifest of all HTML attributes that we require inside __DEV__ for the unknown property hooks.

I've also pushed a new update:

  • I removed the HTML, SVG, and ARIA configs, consolidating all of their rules into the DOM Property config. This actually ends up saving some space post GZIP and avoids needing to enumerate over property configs at boot.
  • I've removed boolean checks for features like isNumeric and isBoolean in favor of object maps for properties that need those settings. This is another experiment to evaluate the performance trade off.

All in all, even if we manually specify all configuration options for every single attribute, the GZIP size ends up being rough the same (or a bit lower) than enumerating over individual lists.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 20, 2017

Member

Please don't run the record-tests script. For... reasons.

Member

acdlite commented Jul 20, 2017

Please don't run the record-tests script. For... reasons.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 1, 2017

Collaborator

Hmm. Server tests fail, I think, because we removed the event plugins from server rendering, so they don't know what attribute names are events. Now that there's no whitelist to stop onchange, it shows up as an attribute when producing server markup.

So this check is out of date:
https://github.com/facebook/react/blob/master/src/renderers/shared/server/ReactPartialRenderer.js#L275

@gaearon I think I'm getting those lovey warnings you just added. Would you consider this a separate bug? It looks like that condition is checking for no reason. I'll do some more digging and possibly file an issue

Collaborator

nhunzaker commented Aug 1, 2017

Hmm. Server tests fail, I think, because we removed the event plugins from server rendering, so they don't know what attribute names are events. Now that there's no whitelist to stop onchange, it shows up as an attribute when producing server markup.

So this check is out of date:
https://github.com/facebook/react/blob/master/src/renderers/shared/server/ReactPartialRenderer.js#L275

@gaearon I think I'm getting those lovey warnings you just added. Would you consider this a separate bug? It looks like that condition is checking for no reason. I'll do some more digging and possibly file an issue

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 2, 2017

Member

Which warnings?

Member

gaearon commented Aug 2, 2017

Which warnings?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 2, 2017

Collaborator

@gaearon I'm possibly wrong, sorry for the noise if so:

Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting. React injected new markup to compensate which works but you have lost many of the benefits of server rendering. Instead, figure out why the markup being generated is different on the client or server:
       (client) heckbox" checked="" data-reactroot="" da
       (server) heckbox" checked="" onchange="function (
Collaborator

nhunzaker commented Aug 2, 2017

@gaearon I'm possibly wrong, sorry for the noise if so:

Warning: React attempted to reuse markup in a container but the checksum was invalid. This generally means that you are using server rendering and the markup generated on the server was not what the client was expecting. React injected new markup to compensate which works but you have lost many of the benefits of server rendering. Instead, figure out why the markup being generated is different on the client or server:
       (client) heckbox" checked="" data-reactroot="" da
       (server) heckbox" checked="" onchange="function (

nhunzaker added some commits Apr 8, 2016

Strip whitelist. Move casing warnings to dev tools
Configures DOMPropertyOperations to allow all attributes to pass
through. It maintains the whitelist only for attributes that need
special behavior.

- Lowercase props during static markup generation
- Remove ARIA property config
- Migrate over a few more SVG properties to the warning list
- Migrate over rules for DOM Fiber. Update tests
- Address test failure after rebase
- Move event filter to top level condition in property loop
- Handle newer fiber and server test updates
- Add complete original whitelist to UnknownDOMPropertyHook
- Use default warning count for SSR property warning tests
- Prevent warnings for property names that are already correct
- Add full SVG property list to ReactDOMUnknownPropertyHook

Revert property lists, add custom attribute feature flag

Turns out I had this backwards. This commit reverts the slash and burn
of the property configs and replaces the custom attribute checks with
a new predicate on DOMProperty that determines if an attribute may be
written.

- Properly control for custom attributes in ReactDOMFiberComponent
Remove all "0" attributes. Remove custom attribute regex pattern.
- Removes the entire ARIA config
- Ignores ARIA tests
- Ignores some missing attribute tests
- Ignores DOM feature flag tests
- Consolidate property configs.
- Eliminate the need to enumerate over property lists
- Simplify operations
@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 2, 2017

Collaborator

Build passes with minor modifications to the test suite for custom attributes. I'm happy with this as a proof of concept. Next I'll do some analysis (I've also updated the description with an outline and initial thoughts).

Collaborator

nhunzaker commented Aug 2, 2017

Build passes with minor modifications to the test suite for custom attributes. I'm happy with this as a proof of concept. Next I'll do some analysis (I've also updated the description with an outline and initial thoughts).

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Aug 2, 2017

Collaborator

@trueadm or @spicyj I want to evaluate possible performance improvements assigning/updating attributes. Is the benchmark conducted by yarn bench a good test case, or do you think it would be better to roll my own?

Collaborator

nhunzaker commented Aug 2, 2017

@trueadm or @spicyj I want to evaluate possible performance improvements assigning/updating attributes. Is the benchmark conducted by yarn bench a good test case, or do you think it would be better to roll my own?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 2, 2017

Member

yarn bench is a reasonable start.

Member

gaearon commented Aug 2, 2017

yarn bench is a reasonable start.

// Do not assign reserved properties or functions. Event handlers should
// not exist in markup.
isWriteable(name, value) {
return !DOMProperty.isReservedProp(name) && typeof value !== 'function';

This comment has been minimized.

@nhunzaker

nhunzaker Aug 2, 2017

Collaborator

⚠️ ⚠️ ⚠️ This typeof value === 'function' prevents properties with a function value type from being written to markup. Chances are that this isn't great. I'm working through how we might better track allowed event handlers and omit them for server renders (this could probably be PR'ed separately to master if someone else doesn't get to it first)

@nhunzaker

nhunzaker Aug 2, 2017

Collaborator

⚠️ ⚠️ ⚠️ This typeof value === 'function' prevents properties with a function value type from being written to markup. Chances are that this isn't great. I'm working through how we might better track allowed event handlers and omit them for server renders (this could probably be PR'ed separately to master if someone else doesn't get to it first)

@nhunzaker nhunzaker closed this Aug 8, 2017

@nhunzaker nhunzaker deleted the attribute-spike branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment