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

Warn about conflicting style values during updates #14181

Merged
merged 1 commit into from Nov 9, 2018

Conversation

@sophiebits
Copy link
Collaborator

sophiebits commented Nov 9, 2018

This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering {background, backgroundSize} with unchanging values, for example. But once you remove either one or change background (without changing backgroundSize at the same time), that's bad news. So we warn.

Fixes #6348.

@sophiebits sophiebits requested review from bvaughn and gaearon Nov 9, 2018
@sophiebits

This comment has been minimized.

Copy link
Collaborator Author

sophiebits commented Nov 9, 2018

@bvaughn This is probably one of the only bugs that both you (#8689) and I (#6348 (comment)) have both run into. 😂 Hope this warning helps.

@sizebot

This comment has been minimized.

Copy link

sizebot commented Nov 9, 2018

Details of bundled changes.

Comparing: 2dd4ba1...399ac96

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +1.2% +1.3% 709.41 KB 718.15 KB 163.99 KB 166.13 KB UMD_DEV
react-dom.development.js +1.2% +1.3% 704.72 KB 713.46 KB 162.61 KB 164.76 KB NODE_DEV
ReactDOM-dev.js +1.3% +1.4% 725.35 KB 734.86 KB 163.71 KB 165.93 KB FB_WWW_DEV

Generated by 🚫 dangerJS

Copy link
Contributor

bvaughn left a comment

Oh hey, this bug.

* {font: 'foo', fontVariant: 'bar'} -> {font: 'foo'}
* becomes .style.fontVariant = ''
*/
export function validateShorthandPropertyCollisionInDev(

This comment has been minimized.

Copy link
@bvaughn

bvaughn Nov 9, 2018

Contributor

Seems like this could get noisy if we don't do any kind of de-duping. I wonder if this is something you considered and ruled out?

Guess de-duping would have a small impact on tests too.

This comment has been minimized.

Copy link
@sophiebits

sophiebits Nov 9, 2018

Author Collaborator

I thought about it but I don't think this will be very common, it is a bad bug, and it is pretty easy to fix – so I think it's OK not to.


expect(() =>
ReactDOM.render(
<div style={{font: 'qux', fontStyle: 'baz'}} />,

This comment has been minimized.

Copy link
@bvaughn

bvaughn Nov 9, 2018

Contributor

Ugh, this is subtle. Had to read these tests a couple times. Nice to have a warning for it.

This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes #6348.
@sophiebits sophiebits force-pushed the sophiebits:css-shorthand-warn branch from 84bcf8c to 399ac96 Nov 9, 2018
@bvaughn
bvaughn approved these changes Nov 9, 2018
@sophiebits sophiebits merged commit 8ae867e into facebook:master Nov 9, 2018
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
sophiebits added a commit to sophiebits/react that referenced this pull request Nov 10, 2018
I figured out a simpler way to do facebook#14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
sophiebits added a commit that referenced this pull request Nov 10, 2018
I figured out a simpler way to do #14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
@gaearon gaearon mentioned this pull request Nov 13, 2018
acdlite added a commit to acdlite/react that referenced this pull request Nov 15, 2018
Disables the recently introduced (facebook#14181) warning for shorthand
CSS property collisions by wrapping in a feature flag. Let's hold off
shipping this until at least the next minor.
acdlite added a commit that referenced this pull request Nov 15, 2018
Disables the recently introduced (#14181) warning for shorthand
CSS property collisions by wrapping in a feature flag. Let's hold off
shipping this until at least the next minor.
jetoneza added a commit to jetoneza/react that referenced this pull request Jan 23, 2019
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes facebook#6348.
jetoneza added a commit to jetoneza/react that referenced this pull request Jan 23, 2019
I figured out a simpler way to do facebook#14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
jetoneza added a commit to jetoneza/react that referenced this pull request Jan 23, 2019
…ok#14245)

Disables the recently introduced (facebook#14181) warning for shorthand
CSS property collisions by wrapping in a feature flag. Let's hold off
shipping this until at least the next minor.
n8schloss added a commit to n8schloss/react that referenced this pull request Jan 31, 2019
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes facebook#6348.
n8schloss added a commit to n8schloss/react that referenced this pull request Jan 31, 2019
I figured out a simpler way to do facebook#14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
n8schloss added a commit to n8schloss/react that referenced this pull request Jan 31, 2019
…ok#14245)

Disables the recently introduced (facebook#14181) warning for shorthand
CSS property collisions by wrapping in a feature flag. Let's hold off
shipping this until at least the next minor.
iyegoroff added a commit to iyegoroff/react that referenced this pull request Mar 9, 2019
This is one of the most insidious quirks of React DOM that people run into. Now we warn when we think an update is dangerous.

We still allow rendering `{background, backgroundSize}` with unchanging values, for example. But once you remove either one or change `background` (without changing `backgroundSize` at the same time), that's bad news. So we warn.

Fixes facebook#6348.
iyegoroff added a commit to iyegoroff/react that referenced this pull request Mar 9, 2019
I figured out a simpler way to do facebook#14181. It does allocate some but I think that's OK. Time complexity might even be better since we avoid the nested loops the old one had.
iyegoroff added a commit to iyegoroff/react that referenced this pull request Mar 9, 2019
…ok#14245)

Disables the recently introduced (facebook#14181) warning for shorthand
CSS property collisions by wrapping in a feature flag. Let's hold off
shipping this until at least the next minor.
This was referenced Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.