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

Wrap shorthand CSS property collision warning in feature flag #14245

Merged
merged 1 commit into from Nov 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
135 changes: 0 additions & 135 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
Expand Up @@ -146,141 +146,6 @@ describe('ReactDOMComponent', () => {
}
});

it('should warn for conflicting CSS shorthand updates', () => {
const container = document.createElement('div');
ReactDOM.render(
<div style={{font: 'foo', fontStyle: 'bar'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{font: 'foo'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (fontStyle) ' +
'when a conflicting property is set (font) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);

// These updates are OK and don't warn:
ReactDOM.render(
<div style={{font: 'qux', fontStyle: 'bar'}} />,
container,
);
ReactDOM.render(
<div style={{font: 'foo', fontStyle: 'baz'}} />,
container,
);

expect(() =>
ReactDOM.render(
<div style={{font: 'qux', fontStyle: 'baz'}} />,
container,
),
).toWarnDev(
'Warning: Updating a style property during rerender (font) when ' +
'a conflicting property is set (fontStyle) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);
expect(() =>
ReactDOM.render(<div style={{fontStyle: 'baz'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (font) when ' +
'a conflicting property is set (fontStyle) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);

// A bit of a special case: backgroundPosition isn't technically longhand
// (it expands to backgroundPosition{X,Y} but so does background)
ReactDOM.render(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{background: 'yellow'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender ' +
'(backgroundPosition) when a conflicting property is set ' +
"(background) can lead to styling bugs. To avoid this, don't mix " +
'shorthand and non-shorthand properties for the same value; ' +
'instead, replace the shorthand with separate values.' +
'\n in div (at **)',
);
ReactDOM.render(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
// But setting them at the same time is OK:
ReactDOM.render(
<div style={{background: 'green', backgroundPosition: 'top'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{backgroundPosition: 'top'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (background) ' +
'when a conflicting property is set (backgroundPosition) can lead ' +
"to styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);

// A bit of an even more special case: borderLeft and borderStyle overlap.
ReactDOM.render(
<div style={{borderStyle: 'dotted', borderLeft: '1px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(
<div style={{borderLeft: '1px solid red'}} />,
container,
),
).toWarnDev(
'Warning: Removing a style property during rerender (borderStyle) ' +
'when a conflicting property is set (borderLeft) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
expect(() =>
ReactDOM.render(
<div style={{borderStyle: 'dashed', borderLeft: '1px solid red'}} />,
container,
),
).toWarnDev(
'Warning: Updating a style property during rerender (borderStyle) ' +
'when a conflicting property is set (borderLeft) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
// But setting them at the same time is OK:
ReactDOM.render(
<div style={{borderStyle: 'dotted', borderLeft: '2px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{borderStyle: 'dotted'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (borderLeft) ' +
'when a conflicting property is set (borderStyle) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
});

it('should warn for unknown prop', () => {
const container = document.createElement('div');
expect(() =>
Expand Down
@@ -0,0 +1,147 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

describe('ReactDOMShorthandCSSPropertyCollision', () => {
let ReactFeatureFlags;
let React;
let ReactDOM;

beforeEach(() => {
jest.resetModules();
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.warnAboutShorthandPropertyCollision = true;
React = require('react');
ReactDOM = require('react-dom');
});

it('should warn for conflicting CSS shorthand updates', () => {
const container = document.createElement('div');
ReactDOM.render(<div style={{font: 'foo', fontStyle: 'bar'}} />, container);
expect(() =>
ReactDOM.render(<div style={{font: 'foo'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (fontStyle) ' +
'when a conflicting property is set (font) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);

// These updates are OK and don't warn:
ReactDOM.render(<div style={{font: 'qux', fontStyle: 'bar'}} />, container);
ReactDOM.render(<div style={{font: 'foo', fontStyle: 'baz'}} />, container);

expect(() =>
ReactDOM.render(
<div style={{font: 'qux', fontStyle: 'baz'}} />,
container,
),
).toWarnDev(
'Warning: Updating a style property during rerender (font) when ' +
'a conflicting property is set (fontStyle) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);
expect(() =>
ReactDOM.render(<div style={{fontStyle: 'baz'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (font) when ' +
'a conflicting property is set (fontStyle) can lead to styling ' +
"bugs. To avoid this, don't mix shorthand and non-shorthand " +
'properties for the same value; instead, replace the shorthand ' +
'with separate values.' +
'\n in div (at **)',
);

// A bit of a special case: backgroundPosition isn't technically longhand
// (it expands to backgroundPosition{X,Y} but so does background)
ReactDOM.render(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{background: 'yellow'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender ' +
'(backgroundPosition) when a conflicting property is set ' +
"(background) can lead to styling bugs. To avoid this, don't mix " +
'shorthand and non-shorthand properties for the same value; ' +
'instead, replace the shorthand with separate values.' +
'\n in div (at **)',
);
ReactDOM.render(
<div style={{background: 'yellow', backgroundPosition: 'center'}} />,
container,
);
// But setting them at the same time is OK:
ReactDOM.render(
<div style={{background: 'green', backgroundPosition: 'top'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{backgroundPosition: 'top'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (background) ' +
'when a conflicting property is set (backgroundPosition) can lead ' +
"to styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);

// A bit of an even more special case: borderLeft and borderStyle overlap.
ReactDOM.render(
<div style={{borderStyle: 'dotted', borderLeft: '1px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{borderLeft: '1px solid red'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (borderStyle) ' +
'when a conflicting property is set (borderLeft) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
expect(() =>
ReactDOM.render(
<div style={{borderStyle: 'dashed', borderLeft: '1px solid red'}} />,
container,
),
).toWarnDev(
'Warning: Updating a style property during rerender (borderStyle) ' +
'when a conflicting property is set (borderLeft) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
// But setting them at the same time is OK:
ReactDOM.render(
<div style={{borderStyle: 'dotted', borderLeft: '2px solid red'}} />,
container,
);
expect(() =>
ReactDOM.render(<div style={{borderStyle: 'dotted'}} />, container),
).toWarnDev(
'Warning: Removing a style property during rerender (borderLeft) ' +
'when a conflicting property is set (borderStyle) can lead to ' +
"styling bugs. To avoid this, don't mix shorthand and " +
'non-shorthand properties for the same value; instead, replace the ' +
'shorthand with separate values.' +
'\n in div (at **)',
);
});
});
6 changes: 6 additions & 0 deletions packages/react-dom/src/shared/CSSPropertyOperations.js
Expand Up @@ -12,6 +12,8 @@ import hyphenateStyleName from './hyphenateStyleName';
import warnValidStyle from './warnValidStyle';
import warning from 'shared/warning';

import {warnAboutShorthandPropertyCollision} from 'shared/ReactFeatureFlags';

/**
* Operations for dealing with CSS properties.
*/
Expand Down Expand Up @@ -123,6 +125,10 @@ export function validateShorthandPropertyCollisionInDev(
styleUpdates,
nextStyles,
) {
if (!warnAboutShorthandPropertyCollision) {
return;
}

if (!nextStyles) {
return;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Expand Up @@ -47,3 +47,5 @@ export const disableInputAttributeSyncing = false;
// These APIs will no longer be "unstable" in the upcoming 16.7 release,
// Control this behavior with a flag to support 16.6 minor releases in the meanwhile.
export const enableStableConcurrentModeAPIs = false;

export const warnAboutShorthandPropertyCollision = false;
Expand Up @@ -23,6 +23,7 @@ export const enableSchedulerTracing = __PROFILE__;
export const enableSuspenseServerRenderer = false;
export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
Expand Up @@ -23,6 +23,7 @@ export const enableSchedulerTracing = __PROFILE__;
export const enableSuspenseServerRenderer = false;
export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Expand Up @@ -28,6 +28,7 @@ export const enableProfilerTimer = __PROFILE__;
export const enableSchedulerTracing = __PROFILE__;
export const enableSuspenseServerRenderer = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Expand Up @@ -23,6 +23,7 @@ export const enableSchedulerTracing = __PROFILE__;
export const enableSuspenseServerRenderer = false;
export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.persistent.js
Expand Up @@ -23,6 +23,7 @@ export const enableSchedulerTracing = __PROFILE__;
export const enableSuspenseServerRenderer = false;
export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Expand Up @@ -23,6 +23,7 @@ export const enableSchedulerTracing = false;
export const enableSuspenseServerRenderer = false;
export const disableInputAttributeSyncing = false;
export const enableStableConcurrentModeAPIs = false;
export const warnAboutShorthandPropertyCollision = false;

// Only used in www builds.
export function addUserTimingListener() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Expand Up @@ -18,6 +18,7 @@ export const {
replayFailedUnitOfWorkWithInvokeGuardedCallback,
warnAboutDeprecatedLifecycles,
disableInputAttributeSyncing,
warnAboutShorthandPropertyCollision,
} = require('ReactFeatureFlags');

// The rest of the flags are static for better dead code elimination.
Expand Down