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

Invalid "Mutating style is deprecated." warning in 0.14.0-rc1? #4877

Closed
moroshko opened this issue Sep 15, 2015 · 24 comments
Closed

Invalid "Mutating style is deprecated." warning in 0.14.0-rc1? #4877

moroshko opened this issue Sep 15, 2015 · 24 comments
Milestone

Comments

@moroshko
Copy link

I get the following warning in 0.14.0-rc1:

Warning: div was passed a style object that has previously been mutated. Mutating style is deprecated. Consider cloning it beforehand. Check the render of Preview. Previous style: {"color":"#747474","backgroundColor":"#9988","fontSize":null,"fontWeight":"300"}. Mutated style: {"color":"#747474","backgroundColor":"#9988","fontSize":null,"fontWeight":"300"}.

I'm confused why this warning appears because the "Previous style" and the "Mutated style" look exactly the same.

To reproduce the issue:

$ git clone git@github.com:moroshko/accessible-colors.git
$ cd accessible-colors
$ git checkout 1d8efda7145ff316b78e60cc0f0ebe113da11367
$ npm install
$ npm start

Then open http://localhost:3000/dist/index.html in a browser, and:

  1. Type "1" in the font size field
  2. Blur that field
    Now you should see the warning in the console.

Why this warning is displayed? How could I fix it?

@sophiebits
Copy link
Collaborator

Looks like your fontSize is getting set to NaN. If you have it as any other value then you won't get the warning.

We can probably have shallowEqual treat two NaNs as the same.

@sophiebits sophiebits added this to the 0.14 milestone Sep 16, 2015
@moroshko
Copy link
Author

Is it NaN or null? The warning message says null.

Yep, I think it makes sense for shallowEqual to consider two nulls (or NaNs) to be the same.

@zpao
Copy link
Member

zpao commented Sep 17, 2015

JSON.stringify({a: NaN}) === '{"a":null}'

So I'm willing to bet @spicyj did his debugging right :)

@moroshko
Copy link
Author

@zpao All I'm saying is that if fontSize is getting NaN, I expect to see NaN in the warning message.

@zpao
Copy link
Member

zpao commented Sep 17, 2015

Sure, I'm saying the language doesn't make that easy for us to do. NaN doesn't have a representation in JSON. We could sub in "NaN" when stringifying and trade one lie for another - maybe that's not as bad as null but it's still a lie since the value isn't a string either.

@moroshko
Copy link
Author

@zpao I'd prefer the 'NaN' lie rather than the null lie. That would give me more clues to debug the potential issue.

@syranide
Copy link
Contributor

We can probably have shallowEqual treat two NaNs as the same.

It should use Object.is for comparison I'd say.

sophiebits added a commit to sophiebits/react that referenced this issue Sep 22, 2015
Fixes facebook#4877. I opted not to change shallowEqual for this since it seems relatively one-off.
sophiebits added a commit to sophiebits/react that referenced this issue Sep 22, 2015
Fixes facebook#4877. I opted not to change shallowEqual for this since it seems relatively one-off.
@programnext
Copy link

Is this fix in 0.14.3? I got a similar issue with react 0.14.3.
Warning: div was passed a style object that has previously been mutated. Mutating style is deprecated. Consider cloning it beforehand. Check the render of Modal.
Previous style: {display: "block", width: "700px", height: {}, marginLeft: NaN, marginTop: -249}.
Mutated style: {display: "block", width: "700px", height: {}, marginLeft: NaN, marginTop: -249}

@waldreiter
Copy link
Contributor

@programnext
The warning is caused by marginLeft being NaN.

@thanzen
Copy link

thanzen commented Jan 4, 2016

I see. Thanks @cody. Is there anyway to workaround the issue since I have no access to marginLeft prop directly.

@jimfb
Copy link
Contributor

jimfb commented Jan 4, 2016

@thanzen We could probably make an error message that is a bit more informative (#5773), but specifying a margin of NaN seems like it should always be an error/warning. Can you elaborate on what you mean by "I have no access to marginLeft prop directly"? If you mean you are using a third-party component that sets the property incorrectly, that component author should fix the issue.

@syranide
Copy link
Contributor

syranide commented Jan 4, 2016

@jimfb Tangential, perhaps we should explicitly warn whenever NaN is passed to a style, it's wrong and very obviously a user error (but not always easily spotted).

@jimfb
Copy link
Contributor

jimfb commented Jan 4, 2016

@syranide #5773 :)

@thanzen
Copy link

thanzen commented Jan 5, 2016

@jimfb, yes, it is a third party component, and I will log the issue under that repo instead.

@essekia
Copy link

essekia commented Feb 9, 2016

I don't have any null or NaN values and still get this error when updating style via Dispatcher -> Store - > changeListening. Any thoughts?

Warning:divwas passed a style object that has previously been mutated. Mutatingstyleis deprecated. Consider cloning it beforehand. Check therenderofnull. Previous style: {width: "500px", float: "left", background: "#fff", marginTop: "10px", marginBottom: "10px", marginLeft: "10px", marginRight: "10px", textAlign: "left"}. Mutated style: {width: "500px", float: "left", background: "#fff", marginTop: "10px", marginBottom: "10px", marginLeft: "10px", marginRight: "10px", textAlign: "left", fontSize: "15px"}.

@sophiebits
Copy link
Collaborator

@essekia You added fontSize: 15px. You need to create a new style object instead of changing the existing one.

@essekia
Copy link

essekia commented Feb 10, 2016

Used jQuery's extend to clone the object and that solved it. Thanks @spicyj

@bradbyte
Copy link

I ran into this issue while using Object.assign. I just set the target to be an empty object so there's a new memory reference.

... style={Object.assign({}, styles.input, this.state.error ? {color: 'red'} : {}) ...

@pansy199211
Copy link

pansy199211 commented Jun 21, 2016

I don't have any NaN or null, and there are no new style added. I get the same warning as above. Anyone have idea about this? thx~

warning.js?0260:44Warning:figurewas passed a style object that has previously been mutated. Mutatingstyleis deprecated. Consider cloning it beforehand. Check therenderofImgFigure. Previous style: {left: 0, top: 0}. Mutated style: {left: 511, top: 272}.

@gaearon
Copy link
Collaborator

gaearon commented Jun 21, 2016

@webpansy

The NaN issue is a false positive of the warning, but in other cases the warning should fire correctly. So check whether you are mutating style that you’re declaring inside of render() method of ImgFigure, just like the warning says. You should not mutate style (or any other prop, really) after it has been returned from render(). It’s hard to say more because I haven’t seen the code, but you might want to ask this on StackOverflow with an example reproducing the problem.

@AoDev
Copy link
Contributor

AoDev commented Jul 25, 2016

@gaearon

Hello. I am using observables with React. Mutating the store is exactly what is being done so that the views subscribe to these changes and re-render if needed. I am using mobx.
I feel like this warning is unnecessary in that case. Cloning the style object from the store just creates more boilerplate and unnecessary operations. The view is observing changes in the styles values and re-render automatically.

For the context: I am moving a character on the screen. Sometimes I reset its position.

These two examples mutate the styles in the store. But in one case, the warning will appear while in the other case it won't. This shows some incoherence.

This does not throw a warning:

// In an action
store.position.x = 100
store.position.y = 100

// In render
const style = {top: `${this.props.position.y}px`, left: `${this.props.position.x}px`}
<div style={style}/>

This throws a warning:

// in an action
store.style.top = '100px'
store.style.left = '100px'

// in render
<div style={store.style} />

Mutation occurred in both cases. Remember that I use observables, so it's valid and expected to simply change values instead of using immutables or "simulate" them by cloning / creating new objects.

What is your opinion about this use case?

PS: I wasn't sure about creating a new issue for this but it felt like a continuation to your last comment. I can create a new issue.

@rickhanlonii
Copy link
Member

rickhanlonii commented Sep 14, 2016

@AoDev the first does not throw a warning because of this line:

const style = { ... };

That's creating a new style object with a new memory reference, so you're actually not passing a mutated object. In the second you're passing store.style which is the same object with the same reference before and after mutation.

You can fix the second with:

<div style={Object.assign({}, store.style)} />

Be sure to do this with care though because of the performance implications (passing props like this to pure children will always force a render, removing the benefits of pure components).

@AoDev
Copy link
Contributor

AoDev commented Sep 17, 2016

@rickhanlonii Thanks, I realised this later but forgot I had commented here.

@feluxe
Copy link

feluxe commented May 5, 2017

@rickhanlonii

Be sure to do this with care though because of the performance implications (passing props like this to pure children will always force a render, removing the benefits of pure components).

I use pure components heavily and this is starting to become a big problem for me.

As far as I can conclude, I have two options:

  1. Create a new object e.g. <div style={Object.assign({}, store.style)} /> to get rid of the warning. This will leave me with unnecessary re-renderings of all affected components.
  2. Create the style object outside the component to get rid of unnecessary re-renderings. This will leave me with the mutation warning.

Is there anything I can do about it?

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

No branches or pull requests