Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Remove unknown props on <div> tag #759

Closed
kylecesmat opened this issue Jul 1, 2016 · 22 comments
Closed

Remove unknown props on <div> tag #759

kylecesmat opened this issue Jul 1, 2016 · 22 comments

Comments

@kylecesmat
Copy link
Contributor

kylecesmat commented Jul 1, 2016

The style-root component is receiving all props passed to it. We should remove unused props as future versions of React React v15.2.0 will throw warning messages.

Warning: Unknown prop `_radiumDidResolveStyles` on <div> tag. Remove this prop from the element. For details, see https://fb.me/react-unknown-prop
    in div (created by StyleRoot)
    in StyleRoot (created by App)
    in App

Line in question - https://github.com/FormidableLabs/radium/blob/master/src/components/style-root.js#L35

Related to this issue - react-bootstrap/react-bootstrap#1994 (comment)

@kylecesmat kylecesmat changed the title Remove unknown props on <div> tag. Remove unknown props on <div> tag Jul 1, 2016
@didierfranc
Copy link

Same issue with my project after React 15.2.0 update

capture d ecran 2016-07-01 a 23 21 17

@NateBrady23
Copy link

Just came here to post this, thanks for taking care of it so fast @kylecesmat !

@alexlande
Copy link
Contributor

alexlande commented Jul 5, 2016

Quote from the related PR:

This doesn't do the trick, unfortunately. This prop is applied with our _cloneElement wrapper, so we can't handle it in the render method. Furthermore, this happens with all elements processed by Radium, not just StyleRoot. Taking discussion back to the open ticket to figure this out one.

A short-term solution would be to make this prop a data attribute to silence the warning. This would render the attribute to the DOM, which is annoying, but that will happen anyway in React 16. Longer term... I'm not sure. We might think about a different mechanism for this behavior.

Edit: Tried this out, and it is extremely noisy rendering a data attribute on every element in an app. Worst case we can do so, but we should try not to.

/cc @ianobermiller

@awkaiser
Copy link

awkaiser commented Jul 6, 2016

Since Radium is using Babel, I was thinking about implementing a fix for this utilizing an ES2015 WeakMap to track which elements had already been processed (sidestepping need for DOM attributes), but that appears to not be allowed by the project configuration. @alexlande, is that an allowance to consider now or would that open a can of worms?

@alexlande
Copy link
Contributor

@awkaiser: I like that idea, but we don't currently require any polyfills (beyond those required by React itself), and I'd like to keep it that way if we can.

@ianobermiller
Copy link
Contributor

weakmap isn't an option because it cannot be polyfilled correctly. We'd have to manually keep track and somehow cleanup on componentWillUnmount.

@tquetano-r7
Copy link

tquetano-r7 commented Jul 11, 2016

Alright this is simplistic, but bear with me.

Currently the only thing _radiumDidResolveStyles does is validate have the styles already been resolved ... do we re-clone the element with newProps or just return the already-cloned element. If we change _radiumDidResolveStyles to a data attribute (something like data-radium-did-resolve-styles), the warning goes away and it works the exact same. It adds a small amount of clutter to the DOM (which can be aided by a less-verbose name), but other than that it solves the problem with minimal code change.

I didn't want to submit my PR if people were gonna blow up at the concept alone, so let me know if this is okay and if so I'll submit straightaway.

@alexlande
Copy link
Contributor

@tquetano-r7: yeah, I floated that previously: #759 (comment)

I don't particularly mind it, and if React 16 came out tomorrow and this was a breaking change we would probably do it. I'm hoping we can land on a different solution that doesn't result in a ton of extra attributes in the DOM, but worst case we'll use data attributes.

@tquetano-r7
Copy link

ah sorry @alexlande i missed that comment. fair enough.

@awkaiser
Copy link

How about data-radium-drs="1" (did resolve styles), data-radium-r (resolved), or even data-radium="1" if this is the only data attribute use for Radium right now? I hear you on DOM noise and don't like the idea any more than @alexlande but, in the interest of clearing this warning, why not go with a data attribute for now and update with a better solution later, rather than holding this particular issue open?

@alexlande
Copy link
Contributor

Using a data- attribute has an actual effect (although a small one) on users, while leaving the error in place has no effect except that it's a little annoying in development mode.

That said, based on the interest in this issue, it seems that the community feels more strongly about having to see the deprecation warning than I do, so maybe it's best that we add a data attribute for now and revisit after.

@alexlande
Copy link
Contributor

This is fixed in 0.18.0

@doctyper
Copy link
Contributor

doctyper commented Jul 19, 2016

@alexlande I'm still seeing this issue using <StyleRoot> with a radiumConfig parameter:

<StyleRoot radiumConfig={{
    userAgent: "..."
>
    <div>Foo</div>
</StyleRoot>
Warning: Unknown prop `radiumConfig` on <div> tag. Remove this prop from the element. For details, see https://fb.me/react-unknown-prop
    in div (created by StyleRoot)
    in StyleRoot (created by ApplicationContext)
    in Perf (created by ApplicationContext)
    in ApplicationContext (created by RouterContext)
    in RouterContext (created by Router)
    in Router (created by Constructor)
    in Constructor

@alexlande
Copy link
Contributor

Ah, thanks @doctyper. That one's an easy fix, at least. PR incoming!

@alexlande
Copy link
Contributor

That's resolved in #787, for any digital archaeologists looking at this thread in the future.

@russellr922
Copy link

@alexlande, I'm still getting this Warning with the latest version of Radium:

Warning: Unknown prop 'radiumConfig' on <div> tag. Remove this prop from the element. For details, see https://fb.me/react-unknown-prop

Any ideas?

"radium": "0.18.2"

@romshiri
Copy link

Me too. Still getting this warning.

@alexlande
Copy link
Contributor

Reopening, I'll try to repro this.

@alexlande alexlande reopened this Sep 19, 2017
@russellr922
Copy link

@alexlande, important to note, I experienced this when using a <div> with a radiumConfig prop. This issue does not occur when using the <StyleRoot>. I'm not sure if @romshiri is experiencing the same, but if so I think this issue can remain closed in favor of properly using the <StyleRoot> component.

@alexlande
Copy link
Contributor

Ah, I see. Thanks for the update @russellr86. @romshiri, can you confirm that you're passing radiumConfig to the StyleRoot component?

@romshiri
Copy link

My bad, I was getting another warning, and it was my fault of using the StyleRoot in appropriately anyway.

@alexlande
Copy link
Contributor

No problem, thanks for checking 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants