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

expose htmlattribute -> htmlAttribute mapping in react-dom #14268

Closed
wants to merge 2 commits into from

Conversation

quantizor
Copy link
Contributor

@quantizor quantizor commented Nov 18, 2018

There are a ton of libraries that reimplement this functionality, leading to unnecessary bundle weight. We can make things easier on the community by simply exporting what react-dom itself uses.

There are a ton of libraries that are reimplementing this functionality
where we can make things easier on the community by simply exporting
what react-dom itself uses for minimal bundle cost
@sizebot
Copy link

sizebot commented Nov 18, 2018

ReactDOM: size: 🔺+11.2%, gzip: 🔺+11.1%

Details of bundled changes.

Comparing: c954efa...f8ef9ee

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% 0.0% 716.38 KB 716.42 KB 165.81 KB 165.84 KB UMD_DEV
react-dom.production.min.js 🔺+11.2% 🔺+11.1% 97.74 KB 108.65 KB 31.81 KB 35.36 KB UMD_PROD
react-dom.profiling.min.js +10.8% +10.9% 100.63 KB 111.54 KB 32.5 KB 36.04 KB UMD_PROFILING
react-dom.development.js 0.0% 0.0% 711.69 KB 711.73 KB 164.43 KB 164.46 KB NODE_DEV
react-dom.production.min.js 🔺+11.2% 🔺+12.2% 97.73 KB 108.64 KB 31.34 KB 35.15 KB NODE_PROD
react-dom.profiling.min.js +10.8% +12.1% 100.72 KB 111.63 KB 31.94 KB 35.8 KB NODE_PROFILING
ReactDOM-dev.js 0.0% 0.0% 731.89 KB 731.94 KB 165.36 KB 165.39 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+4.4% 🔺+7.3% 308.26 KB 321.97 KB 56.77 KB 60.92 KB FB_WWW_PROD
ReactDOM-profiling.js +4.4% +6.9% 315.04 KB 328.75 KB 58.15 KB 62.19 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

@quantizor
Copy link
Contributor Author

quantizor commented Nov 18, 2018

Alternatively if we don't want to make react-dom itself bigger, it could be extracted and moved to a different package or an alternative import similar to react-dom/server

@gaearon
Copy link
Collaborator

gaearon commented Nov 22, 2018

There are a ton of libraries that reimplement this functionality

Why? That seems like the problem.

For example, styled-components reliance on this is a conceptual issue with its API. Not with the implementation.

@quantizor
Copy link
Contributor Author

quantizor commented Nov 22, 2018 via email

@gaearon
Copy link
Collaborator

gaearon commented Nov 22, 2018

Because React warns if you pass through an attribute it doesn’t recognize.

This hasn't been true since React 16. React lets you pass any attributes through.

It does warn for invalid casing of supported attributes. However, you should just let React do its thing, and not try to detect it at a library level.

The root of the issue, to my knowledge, is that some libraries attempt to filter DOM attributes. That's a problem by itself because it doesn't let people pass custom attributes to the DOM. If those libraries don't attempt to do that, then the whole thing is not an issue.

I'm saying that attempting to do filtering before React is a design flaw in these libraries. That they need to fix. We shouldn't be making it more convenient to create more libraries with this design flaw.

Am I missing something?

@quantizor
Copy link
Contributor Author

It does warn for invalid casing of supported attributes. However, you should just let React do its thing, and not try to detect it at a library level.

I would love to not have to, believe me. All of these sort of activities are ultimately workarounds for deficiencies in prop handling via JSX when HOCs are involved. If namespacing could be seriously considered, styled components et all could drop all prop filtering activities except plucking out its own namespace.

@quantizor
Copy link
Contributor Author

This hasn't been true since React 16. React lets you pass any attributes through.

That's also not accurate. This warning is emitted for "linkStyle", for example:

Warning: React does not recognize the linkStyle prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase linkstyle instead. If you accidentally passed it from a parent component, remove it from the DOM element.

@gaearon
Copy link
Collaborator

gaearon commented Nov 23, 2018

This warning is emitted for "linkStyle", for example:

Right, but it tells you what to do. If you pass linkstyle it won’t warn. In either case it will add it to the DOM.

More importantly, this warning occurs for any camelCase name outside of the ones supported by React. I don’t see how exporting a map of known attribute would help you with it.

Maybe let’s start from another end. If React did provide an export like this, what would you do with it? In development and in production.

You describe it as a common need but the only library I consistently see bit by this is styled components — because its API decisions early on relies on there existing a difference between “DOM props” and “other props” which is not the case anymore in React.

@gaearon
Copy link
Collaborator

gaearon commented Nov 23, 2018

Let me try to explain it differently.

React doesn’t depend on this list for production behavior. However, my understanding is that styled components would like to. That’s something we’d prefer to discourage.

It’s a feature that you can do <div something="foo"> and React will pass it through. This was very commonly requested for different integrations with other libraries, trying new attributes without waiting for React to support them, non-standard platform or browser-specific attributes, and other use cases. And it works in React 16.

If a library limits this expressiveness by dividing props into “DOM props” and “non-DOM props” it’s the library’s choice. But it’s not what people expect primitive React components like div to do, and we don’t intend to make it easier to do so.

In fact relying on this list in production turns React supporting any new DOM attribute into a potential breaking change. Such as <StyledDiv goodStuff="foo"> not passing this hypothetical goodStuff through (say it’s used for theming), but then React adds support for it, and now it’s being passed through with potentially different semantics dictated by the DOM. This hazard was greatly reduced since React 16 but the whitelist approach brings it back.

I don’t have a great answer for what Styled Components should do. I think my personal favorite option is to confine all style-only parameterization to a single prop, like variant. If that’s too undesirable I guess you can keep the current whitelist semantics. But I don’t think React should encourage more libraries to make that choice by making it easy.

Hope that makes some sense.

@gaearon gaearon closed this Nov 23, 2018
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.

4 participants