From dd306063850e9ecdb429a5e4231e8f0c548ca841 Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Wed, 20 Mar 2019 00:11:30 -0700 Subject: [PATCH 01/16] Add ability to extend styles --- package.json | 1 + src/ThemedStyleSheet.js | 22 ++- src/withExtendStyles.jsx | 46 +++++ src/withStyles.jsx | 31 +++- test/withExtendStyles_test.jsx | 325 +++++++++++++++++++++++++++++++++ test/withStyles_test.jsx | 162 +++++++++++++++- 6 files changed, 565 insertions(+), 22 deletions(-) create mode 100644 src/withExtendStyles.jsx create mode 100644 test/withExtendStyles_test.jsx diff --git a/package.json b/package.json index 2c6b0d0b..6655ce5f 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "react-with-direction": "^1.1.0" }, "dependencies": { + "deepmerge": "^3.2.0", "hoist-non-react-statics": "^3.2.1", "object.assign": "^4.1.0", "prop-types": "^15.6.2", diff --git a/src/ThemedStyleSheet.js b/src/ThemedStyleSheet.js index ac29acd4..049aa147 100644 --- a/src/ThemedStyleSheet.js +++ b/src/ThemedStyleSheet.js @@ -1,3 +1,5 @@ +import deepmerge from 'deepmerge'; + let styleInterface; let styleTheme; @@ -9,17 +11,25 @@ function registerInterface(interfaceToRegister) { styleInterface = interfaceToRegister; } -function create(makeFromTheme, createWithDirection) { - const styles = createWithDirection(makeFromTheme(styleTheme)); +function extendStyles(makeFromTheme, extendFromTheme = []) { + const baseStyle = makeFromTheme(styleTheme); + const extendedStyles = extendFromTheme.map(extendStyleFn => extendStyleFn(styleTheme)); + + return deepmerge.all([baseStyle, ...extendedStyles]); +} + +function create(makeFromTheme, createWithDirection, extendFromTheme) { + const styles = createWithDirection(extendStyles(makeFromTheme, extendFromTheme)); + return () => styles; } -function createLTR(makeFromTheme) { - return create(makeFromTheme, styleInterface.createLTR || styleInterface.create); +function createLTR(makeFromTheme, extendFromTheme) { + return create(makeFromTheme, styleInterface.createLTR || styleInterface.create, extendFromTheme); } -function createRTL(makeFromTheme) { - return create(makeFromTheme, styleInterface.createRTL || styleInterface.create); +function createRTL(makeFromTheme, extendFromTheme) { + return create(makeFromTheme, styleInterface.createRTL || styleInterface.create, extendFromTheme); } function get() { diff --git a/src/withExtendStyles.jsx b/src/withExtendStyles.jsx new file mode 100644 index 00000000..bdbce7a3 --- /dev/null +++ b/src/withExtendStyles.jsx @@ -0,0 +1,46 @@ +/* eslint react/forbid-foreign-prop-types: off */ + +import React from 'react'; +import PropTypes from 'prop-types'; +import hoistNonReactStatics from 'hoist-non-react-statics'; + +export default function withExtendStyles( + extendStyleFn, + { + extendStyleFnPropName = '_extendStyleFn', + } = {}, +) { + return function withExtendStylesHOC(WrappedComponent) { + const wrappedComponentName = WrappedComponent.displayName + || WrappedComponent.name + || 'Component'; + + const WithExtendStyles = ({ [extendStyleFnPropName]: extendStyleFns, ...rest }) => ( + + ); + + WithExtendStyles.WrappedComponent = WrappedComponent; + WithExtendStyles.displayName = `withExtendStyles(${wrappedComponentName})`; + if (WrappedComponent.propTypes) { + WithExtendStyles.propTypes = { + [extendStyleFnPropName]: PropTypes.arrayOf( + PropTypes.oneOfType([PropTypes.func, PropTypes.object]), + ), + ...WrappedComponent.propTypes, + }; + } + if (WrappedComponent.defaultProps) { + WithExtendStyles.defaultProps = { ...WrappedComponent.defaultProps }; + } + + return hoistNonReactStatics(WithExtendStyles, WrappedComponent); + }; +} diff --git a/src/withStyles.jsx b/src/withStyles.jsx index 79ad8650..e580732c 100644 --- a/src/withStyles.jsx +++ b/src/withStyles.jsx @@ -7,6 +7,7 @@ import hoistNonReactStatics from 'hoist-non-react-statics'; import { CHANNEL, DIRECTIONS } from 'react-with-direction/dist/constants'; import brcastShape from 'react-with-direction/dist/proptypes/brcast'; +import withExtendStylesHoc from './withExtendStyles'; import ThemedStyleSheet from './ThemedStyleSheet'; // Add some named exports to assist in upgrading and for convenience @@ -15,7 +16,9 @@ export const withStylesPropTypes = { styles: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types theme: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types css: PropTypes.func.isRequired, + _extendStyleFn: PropTypes.arrayOf(PropTypes.func), }; +export const withExtendStyles = withExtendStylesHoc; const EMPTY_STYLES = {}; const EMPTY_STYLES_FN = () => EMPTY_STYLES; @@ -44,6 +47,7 @@ export function withStyles( stylesPropName = 'styles', themePropName = 'theme', cssPropName = 'css', + extendStyleFnPropName = '_extendStyleFn', flushBefore = false, pureComponent = false, } = {}, @@ -66,7 +70,7 @@ export function withStyles( : currentThemeRTL; } - function getStyleDef(direction, wrappedComponentName) { + function getStyleDef(direction, wrappedComponentName, extendStyleFns) { const currentTheme = getCurrentTheme(direction); let styleDef = direction === DIRECTIONS.LTR ? styleDefLTR @@ -93,14 +97,14 @@ export function withStyles( if (isRTL) { styleDefRTL = styleFn - ? ThemedStyleSheet.createRTL(styleFn) + ? ThemedStyleSheet.createRTL(styleFn, extendStyleFns) : EMPTY_STYLES_FN; currentThemeRTL = registeredTheme; styleDef = styleDefRTL; } else { styleDefLTR = styleFn - ? ThemedStyleSheet.createLTR(styleFn) + ? ThemedStyleSheet.createLTR(styleFn, extendStyleFns) : EMPTY_STYLES_FN; currentThemeLTR = registeredTheme; @@ -124,10 +128,10 @@ export function withStyles( return styleDef; } - function getState(direction, wrappedComponentName) { + function getState(direction, wrappedComponentName, extendStyleFns) { return { resolveMethod: getResolveMethod(direction), - styleDef: getStyleDef(direction, wrappedComponentName), + styleDef: getStyleDef(direction, wrappedComponentName, extendStyleFns), }; } @@ -146,14 +150,16 @@ export function withStyles( ? this.context[CHANNEL].getState() : defaultDirection; - this.state = getState(direction, wrappedComponentName); + this.extendStyleFns = props[extendStyleFnPropName] || []; + + this.state = getState(direction, wrappedComponentName, this.extendStyleFns); } componentDidMount() { if (this.context[CHANNEL]) { // subscribe to future direction changes this.channelUnsubscribe = this.context[CHANNEL].subscribe((direction) => { - this.setState(getState(direction, wrappedComponentName)); + this.setState(getState(direction, wrappedComponentName, this.extendStyleFns)); }); } } @@ -180,10 +186,14 @@ export function withStyles( resolveMethod, styleDef, } = this.state; + const { + [extendStyleFnPropName]: extendStyleFns, + ...rest + } = this.props; return ( { + const defaultTheme = { + color: { + red: '#990000', + blue: '#334CFF', + }, + }; + + let testInterface; + + beforeEach(() => { + testInterface = { + create() {}, + createLTR() {}, + createRTL() {}, + resolve() {}, + resolveLTR() {}, + resolveRTL() {}, + flush: sinon.spy(), + }; + sinon.stub(testInterface, 'create').callsFake(styleHash => styleHash); + sinon.stub(testInterface, 'createLTR').callsFake(styleHash => styleHash); + sinon.stub(testInterface, 'createRTL').callsFake(styleHash => styleHash); + const fakeResolveMethod = styles => ({ + style: styles.reduce((result, style) => Object.assign(result, style)), + }); + sinon.stub(testInterface, 'resolve').callsFake(fakeResolveMethod); + sinon.stub(testInterface, 'resolveLTR') + .callsFake(fakeResolveMethod); + sinon.stub(testInterface, 'resolveRTL') + .callsFake(fakeResolveMethod); + + ThemedStyleSheet.registerTheme(defaultTheme); + ThemedStyleSheet.registerInterface(testInterface); + }); + + afterEach(() => { + sinon.restore(); + }); + + it('creates the styles in a non-directional context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); + render(); + + expect(testInterface.createLTR.callCount).to.equal(1); + }); + + it('creates the styles in an LTR context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); + render(( + + + + )); + expect(testInterface.createLTR.callCount).to.equal(1); + }); + + it('creates the styles in an RTL context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); + render(( + + + + )); + expect(testInterface.createRTL.callCount).to.equal(1); + }); + + it('extends base styles', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({ + container: { + background: 'red', + color: 'blue', + }, + }))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ + container: { + background: 'green', + }, + }))(WrappedComponent); + render( + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + }); + }); + + it('extends base styles (multiple classes)', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({ + container: { + background: 'red', + color: 'blue', + }, + innerContainer: { + background: 'white', + border: '1px solid black', + }, + content: { + fontSize: '25px', + }, + }))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ + container: { + background: 'green', + }, + innerContainer: { + border: '10px solid green', + }, + content: { + fontSize: '12px', + }, + }))(WrappedComponent); + render( + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + innerContainer: { + background: 'white', + border: '10px solid green', + }, + content: { + fontSize: '12px', + }, + }); + }); + + it('extends base styles with multiple layers', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({ + container: { + background: 'red', + color: 'blue', + fontSize: '10px', + }, + }))(MyComponent); + + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ + container: { + background: 'green', + fontSize: '12px', + }, + }))(WrappedComponent); + + const WrappedComponentWithNestedCustomStyles = withExtendStyles(() => ({ + container: { + fontSize: '20px', + }, + }))(WrappedComponentWithExtendedStyles); + + render( + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + fontSize: '20px', + }, + }); + }); + + it('uses original base styles if no extended styles are provided', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({ + container: { + background: 'red', + color: 'blue', + }, + }))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); + render( + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'red', + color: 'blue', + }, + }); + }); + + it('receives the registered theme in the extend style function', (done) => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles((theme) => { + expect(theme).to.equal(defaultTheme); + done(); + return {}; + })(WrappedComponent); + render( + , + ); + }); + + it('allows the extend styles prop name to be customized', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { extendStyleFnPropName: 'foobar' }, + )(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles( + () => ({ + container: { + background: 'pink', + }, + }), + { extendStyleFnPropName: 'foobar' }, + )(WrappedComponent); + render( + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'pink', + color: 'blue', + }, + }); + }); + + it('passes the processed styles to the wrapped component', () => { + function MyComponent({ styles }) { + expect(styles).to.eql({ foo: { color: '#334CFF' } }); + return null; + } + + const WrappedComponent = withStyles(({ color }) => ({ + foo: { + color: color.red, + }, + }))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(({ color }) => ({ + foo: { + color: color.blue, + }, + }))(WrappedComponent); + render(); + }); + + it('has a wrapped displayName', () => { + function MyComponent() { + return null; + } + + const result = withExtendStyles(() => ({}))(MyComponent); + expect(result.displayName).to.equal('withExtendStyles(MyComponent)'); + }); + + it('hoists statics', () => { + function MyComponent() { + return null; + } + MyComponent.foo = 'bar'; + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); + expect(WrappedComponentWithExtendedStyles.foo).to.equal('bar'); + }); +}); diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index a5a2172c..52e3c771 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -200,6 +200,158 @@ describe('withStyles()', () => { }); }); + describe('extendStyleFn', () => { + it('extends styles in a non-directional context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({ + container: { + background: 'red', + color: 'blue', + }, + }))(MyComponent); + render( + ({ + container: { + background: 'green', + }, + }), + ]} + />, + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + }); + }); + + it('extends styles in an LTR context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({ + container: { + background: 'red', + color: 'blue', + }, + }))(MyComponent); + render( + + ({ + container: { + background: 'green', + }, + }), + ]} + /> + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + }); + }); + + it('extends styles in an RTL context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({ + container: { + background: 'red', + color: 'blue', + }, + }))(MyComponent); + render( + + ({ + container: { + background: 'green', + }, + }), + ]} + /> + , + ); + + expect(testInterface.createRTL.callCount).to.equal(1); + expect(testInterface.createRTL.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + }); + }); + + it('receives the registered theme in the extend style function', (done) => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + shallow( + { + expect(theme).to.equal(defaultTheme); + done(); + return {}; + }, + ]} + />, + ); + }); + + it('allows the extendStyleFn prop name to be customized', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({}), + { + extendStyleFnPropName: 'newExtendStyleFn', + }, + )(MyComponent); + shallow( + ({ + container: { + background: 'green', + }, + }), + ]} + />, + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + }, + }); + }); + }); + it('has a wrapped displayName', () => { function MyComponent() { return null; @@ -340,16 +492,12 @@ describe('withStyles()', () => { }))(MyComponent); // copied - const expectedPropTypes = { ...MyComponent.propTypes }; - delete expectedPropTypes.styles; - delete expectedPropTypes.theme; - delete expectedPropTypes.css; - expect(Wrapped.propTypes).to.eql(expectedPropTypes); - expect(MyComponent.propTypes).to.include.keys('styles', 'theme'); + expect(Wrapped.propTypes).to.include.keys('foo', '_extendStyleFn'); + expect(MyComponent.propTypes).to.include.keys('styles', 'theme', 'css'); expect(Wrapped.defaultProps).to.eql(MyComponent.defaultProps); - // cloned + // // cloned expect(Wrapped.propTypes).not.to.equal(MyComponent.propTypes); expect(Wrapped.defaultProps).not.to.equal(MyComponent.defaultProps); }); From 9ed0c4c584771170de585578e4ddefdcb8dc810b Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Wed, 20 Mar 2019 00:11:30 -0700 Subject: [PATCH 02/16] Validate extending styles --- src/ThemedStyleSheet.js | 59 +++++++++-- src/withStyles.jsx | 5 +- test/withExtendStyles_test.jsx | 176 +++++++++++++++++++++++++++------ test/withStyles_test.jsx | 99 ++++++++++++++++--- 4 files changed, 283 insertions(+), 56 deletions(-) diff --git a/src/ThemedStyleSheet.js b/src/ThemedStyleSheet.js index 049aa147..5eac231a 100644 --- a/src/ThemedStyleSheet.js +++ b/src/ThemedStyleSheet.js @@ -11,25 +11,68 @@ function registerInterface(interfaceToRegister) { styleInterface = interfaceToRegister; } -function extendStyles(makeFromTheme, extendFromTheme = []) { +function validateStyle(style, extendableStyles, currentPath = '') { + if (style === null || Array.isArray(style) || typeof style !== 'object') { + return; + } + + const styleKeys = Object.keys(style); + if (styleKeys.length) { + styleKeys.forEach((styleKey) => { + const path = `${currentPath}.${styleKey}`; + const isValid = extendableStyles[styleKey]; + if (!isValid) { + throw new Error( + `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` + + 'the component\'s "extendableStyles" option.', + ); + } + validateStyle(style[styleKey], extendableStyles[styleKey], path); + }); + } +} + +function validateAndMergeStyles(makeFromTheme, extendFromTheme = [], extendableStyles) { const baseStyle = makeFromTheme(styleTheme); - const extendedStyles = extendFromTheme.map(extendStyleFn => extendStyleFn(styleTheme)); + const extendedStyles = extendFromTheme.map((extendStyleFn) => { + const style = extendStyleFn(styleTheme); + + if (process.env.NODE_ENV !== 'production') { + validateStyle(style, extendableStyles); + } + + return style; + }); return deepmerge.all([baseStyle, ...extendedStyles]); } -function create(makeFromTheme, createWithDirection, extendFromTheme) { - const styles = createWithDirection(extendStyles(makeFromTheme, extendFromTheme)); +function create(makeFromTheme, createWithDirection, extendFromTheme, extendableStyles) { + const styles = createWithDirection(validateAndMergeStyles( + makeFromTheme, + extendFromTheme, + extendableStyles, + )); return () => styles; } -function createLTR(makeFromTheme, extendFromTheme) { - return create(makeFromTheme, styleInterface.createLTR || styleInterface.create, extendFromTheme); +function createLTR(makeFromTheme, extendFromTheme, extendableStyles) { + return create( + makeFromTheme, + styleInterface.createLTR || styleInterface.create, + extendFromTheme, + extendableStyles, + ); } -function createRTL(makeFromTheme, extendFromTheme) { - return create(makeFromTheme, styleInterface.createRTL || styleInterface.create, extendFromTheme); +function createRTL(makeFromTheme, extendFromTheme, extendableStyles) { + return create( + makeFromTheme, + styleInterface.createRTL || styleInterface.create, + extendFromTheme, + extendableStyles, + ); } function get() { diff --git a/src/withStyles.jsx b/src/withStyles.jsx index e580732c..4b58e479 100644 --- a/src/withStyles.jsx +++ b/src/withStyles.jsx @@ -48,6 +48,7 @@ export function withStyles( themePropName = 'theme', cssPropName = 'css', extendStyleFnPropName = '_extendStyleFn', + extendableStyles = {}, flushBefore = false, pureComponent = false, } = {}, @@ -97,14 +98,14 @@ export function withStyles( if (isRTL) { styleDefRTL = styleFn - ? ThemedStyleSheet.createRTL(styleFn, extendStyleFns) + ? ThemedStyleSheet.createRTL(styleFn, extendStyleFns, extendableStyles) : EMPTY_STYLES_FN; currentThemeRTL = registeredTheme; styleDef = styleDefRTL; } else { styleDefLTR = styleFn - ? ThemedStyleSheet.createLTR(styleFn, extendStyleFns) + ? ThemedStyleSheet.createLTR(styleFn, extendStyleFns, extendableStyles) : EMPTY_STYLES_FN; currentThemeLTR = registeredTheme; diff --git a/test/withExtendStyles_test.jsx b/test/withExtendStyles_test.jsx index 0f35e05d..7d8010b9 100644 --- a/test/withExtendStyles_test.jsx +++ b/test/withExtendStyles_test.jsx @@ -95,12 +95,21 @@ describe('withExtendStyles()', () => { return null; } - const WrappedComponent = withStyles(() => ({ - container: { - background: 'red', - color: 'blue', + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, }, - }))(MyComponent); + )(MyComponent); const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ container: { background: 'green', @@ -124,19 +133,34 @@ describe('withExtendStyles()', () => { return null; } - const WrappedComponent = withStyles(() => ({ - container: { - background: 'red', - color: 'blue', - }, - innerContainer: { - background: 'white', - border: '1px solid black', - }, - content: { - fontSize: '25px', + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + innerContainer: { + background: 'white', + border: '1px solid black', + }, + content: { + fontSize: '25px', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + innerContainer: { + border: true, + }, + content: { + fontSize: true, + }, + }, }, - }))(MyComponent); + )(MyComponent); const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ container: { background: 'green', @@ -173,13 +197,23 @@ describe('withExtendStyles()', () => { return null; } - const WrappedComponent = withStyles(() => ({ - container: { - background: 'red', - color: 'blue', - fontSize: '10px', + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + fontSize: '10px', + }, + }), + { + extendableStyles: { + container: { + background: true, + fontSize: true, + }, + }, }, - }))(MyComponent); + )(MyComponent); const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ container: { @@ -188,14 +222,14 @@ describe('withExtendStyles()', () => { }, }))(WrappedComponent); - const WrappedComponentWithNestedCustomStyles = withExtendStyles(() => ({ + const WrappedComponentWithNestedExtendedStyles = withExtendStyles(() => ({ container: { fontSize: '20px', }, }))(WrappedComponentWithExtendedStyles); render( - , + , ); expect(testInterface.createLTR.callCount).to.equal(1); @@ -208,7 +242,71 @@ describe('withExtendStyles()', () => { }); }); - it('uses original base styles if no extended styles are provided', () => { + it('throws an error if an invalid extending style is provided', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + fontSize: '10px', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, + }, + )(MyComponent); + + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ + container: { + // fontSize is invalid + fontSize: '12px', + }, + }))(WrappedComponent); + + expect(() => render( + , + )).to.throw(); + }); + + it('throws an error if extendableStyles is not defined, and an extending style is provided', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + fontSize: '10px', + }, + }), + { + // no extendableStyles + }, + )(MyComponent); + + const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ + container: { + // fontSize is invalid + fontSize: '12px', + }, + }))(WrappedComponent); + + expect(() => render( + , + )).to.throw(); + }); + + it('uses original base styles if no extending styles are provided', () => { function MyComponent() { return null; } @@ -261,7 +359,14 @@ describe('withExtendStyles()', () => { color: 'blue', }, }), - { extendStyleFnPropName: 'foobar' }, + { + extendStyleFnPropName: 'foobar', + extendableStyles: { + container: { + background: true, + }, + }, + }, )(MyComponent); const WrappedComponentWithExtendedStyles = withExtendStyles( () => ({ @@ -290,11 +395,20 @@ describe('withExtendStyles()', () => { return null; } - const WrappedComponent = withStyles(({ color }) => ({ - foo: { - color: color.red, + const WrappedComponent = withStyles( + ({ color }) => ({ + foo: { + color: color.red, + }, + }), + { + extendableStyles: { + foo: { + color: true, + }, + }, }, - }))(MyComponent); + )(MyComponent); const WrappedComponentWithExtendedStyles = withExtendStyles(({ color }) => ({ foo: { color: color.blue, diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index 52e3c771..e38b7e7e 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -206,12 +206,21 @@ describe('withStyles()', () => { return null; } - const WrappedComponent = withStyles(() => ({ - container: { - background: 'red', - color: 'blue', + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, }, - }))(MyComponent); + )(MyComponent); render( { return null; } - const WrappedComponent = withStyles(() => ({ - container: { - background: 'red', - color: 'blue', + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, }, - }))(MyComponent); + )(MyComponent); render( { return null; } - const WrappedComponent = withStyles(() => ({ - container: { - background: 'red', - color: 'blue', + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, }, - }))(MyComponent); + )(MyComponent); render( { }); }); + it('throws an error if an invalid style is extending', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, + }, + )(MyComponent); + + expect(() => render( + + ({ + container: { + // color is invalid + color: 'green', + }, + }), + ]} + /> + , + )).to.throw(); + }); + it('receives the registered theme in the extend style function', (done) => { function MyComponent() { return null; @@ -329,6 +393,11 @@ describe('withStyles()', () => { () => ({}), { extendStyleFnPropName: 'newExtendStyleFn', + extendableStyles: { + container: { + background: true, + }, + }, }, )(MyComponent); shallow( From 4fcfe3f89d913598294eae2001d94cbfe037ee75 Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Thu, 21 Mar 2019 19:16:01 -0700 Subject: [PATCH 03/16] Add static property to extend styles --- src/ThemedStyleSheet.js | 65 +---- src/extendStyles.jsx | 42 +++ src/withExtendStyles.jsx | 46 ---- src/withStyles.jsx | 46 ++-- test/withExtendStyles_test.jsx | 439 -------------------------------- test/withStyles_test.jsx | 452 +++++++++++++++++---------------- 6 files changed, 301 insertions(+), 789 deletions(-) create mode 100644 src/extendStyles.jsx delete mode 100644 src/withExtendStyles.jsx delete mode 100644 test/withExtendStyles_test.jsx diff --git a/src/ThemedStyleSheet.js b/src/ThemedStyleSheet.js index 5eac231a..ac29acd4 100644 --- a/src/ThemedStyleSheet.js +++ b/src/ThemedStyleSheet.js @@ -1,5 +1,3 @@ -import deepmerge from 'deepmerge'; - let styleInterface; let styleTheme; @@ -11,68 +9,17 @@ function registerInterface(interfaceToRegister) { styleInterface = interfaceToRegister; } -function validateStyle(style, extendableStyles, currentPath = '') { - if (style === null || Array.isArray(style) || typeof style !== 'object') { - return; - } - - const styleKeys = Object.keys(style); - if (styleKeys.length) { - styleKeys.forEach((styleKey) => { - const path = `${currentPath}.${styleKey}`; - const isValid = extendableStyles[styleKey]; - if (!isValid) { - throw new Error( - `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` - + 'the component\'s "extendableStyles" option.', - ); - } - validateStyle(style[styleKey], extendableStyles[styleKey], path); - }); - } -} - -function validateAndMergeStyles(makeFromTheme, extendFromTheme = [], extendableStyles) { - const baseStyle = makeFromTheme(styleTheme); - const extendedStyles = extendFromTheme.map((extendStyleFn) => { - const style = extendStyleFn(styleTheme); - - if (process.env.NODE_ENV !== 'production') { - validateStyle(style, extendableStyles); - } - - return style; - }); - - return deepmerge.all([baseStyle, ...extendedStyles]); -} - -function create(makeFromTheme, createWithDirection, extendFromTheme, extendableStyles) { - const styles = createWithDirection(validateAndMergeStyles( - makeFromTheme, - extendFromTheme, - extendableStyles, - )); - +function create(makeFromTheme, createWithDirection) { + const styles = createWithDirection(makeFromTheme(styleTheme)); return () => styles; } -function createLTR(makeFromTheme, extendFromTheme, extendableStyles) { - return create( - makeFromTheme, - styleInterface.createLTR || styleInterface.create, - extendFromTheme, - extendableStyles, - ); +function createLTR(makeFromTheme) { + return create(makeFromTheme, styleInterface.createLTR || styleInterface.create); } -function createRTL(makeFromTheme, extendFromTheme, extendableStyles) { - return create( - makeFromTheme, - styleInterface.createRTL || styleInterface.create, - extendFromTheme, - extendableStyles, - ); +function createRTL(makeFromTheme) { + return create(makeFromTheme, styleInterface.createRTL || styleInterface.create); } function get() { diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx new file mode 100644 index 00000000..a22be7dc --- /dev/null +++ b/src/extendStyles.jsx @@ -0,0 +1,42 @@ +/* eslint react/forbid-foreign-prop-types: off */ + +import deepmerge from 'deepmerge'; + +function validateStyle(style, extendableStyles, currentPath = '') { + // TODO: Only if DEV + if (style === null || Array.isArray(style) || typeof style !== 'object') { + return; + } + + const styleKeys = Object.keys(style); + if (styleKeys.length) { + styleKeys.forEach((styleKey) => { + const path = `${currentPath}.${styleKey}`; + const isValid = extendableStyles[styleKey]; + if (!isValid) { + throw new Error( + `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` + + 'the component\'s "extendableStyles" option.', + ); + } + validateStyle(style[styleKey], extendableStyles[styleKey], path); + }); + } +} + +export default function extendStyles( + baseStyleFn, + extendingStyleFn, + extendableStyles, +) { + return (theme) => { + const baseStyle = baseStyleFn(theme); + const extendingStyle = extendingStyleFn(theme); + + validateStyle(extendingStyle, extendableStyles); + + const styles = deepmerge(baseStyle, extendingStyle); + + return styles; + }; +} diff --git a/src/withExtendStyles.jsx b/src/withExtendStyles.jsx deleted file mode 100644 index bdbce7a3..00000000 --- a/src/withExtendStyles.jsx +++ /dev/null @@ -1,46 +0,0 @@ -/* eslint react/forbid-foreign-prop-types: off */ - -import React from 'react'; -import PropTypes from 'prop-types'; -import hoistNonReactStatics from 'hoist-non-react-statics'; - -export default function withExtendStyles( - extendStyleFn, - { - extendStyleFnPropName = '_extendStyleFn', - } = {}, -) { - return function withExtendStylesHOC(WrappedComponent) { - const wrappedComponentName = WrappedComponent.displayName - || WrappedComponent.name - || 'Component'; - - const WithExtendStyles = ({ [extendStyleFnPropName]: extendStyleFns, ...rest }) => ( - - ); - - WithExtendStyles.WrappedComponent = WrappedComponent; - WithExtendStyles.displayName = `withExtendStyles(${wrappedComponentName})`; - if (WrappedComponent.propTypes) { - WithExtendStyles.propTypes = { - [extendStyleFnPropName]: PropTypes.arrayOf( - PropTypes.oneOfType([PropTypes.func, PropTypes.object]), - ), - ...WrappedComponent.propTypes, - }; - } - if (WrappedComponent.defaultProps) { - WithExtendStyles.defaultProps = { ...WrappedComponent.defaultProps }; - } - - return hoistNonReactStatics(WithExtendStyles, WrappedComponent); - }; -} diff --git a/src/withStyles.jsx b/src/withStyles.jsx index 4b58e479..ef50c5e5 100644 --- a/src/withStyles.jsx +++ b/src/withStyles.jsx @@ -7,18 +7,17 @@ import hoistNonReactStatics from 'hoist-non-react-statics'; import { CHANNEL, DIRECTIONS } from 'react-with-direction/dist/constants'; import brcastShape from 'react-with-direction/dist/proptypes/brcast'; -import withExtendStylesHoc from './withExtendStyles'; +import extendStyles from './extendStyles'; import ThemedStyleSheet from './ThemedStyleSheet'; + // Add some named exports to assist in upgrading and for convenience export const css = ThemedStyleSheet.resolveLTR; export const withStylesPropTypes = { styles: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types theme: PropTypes.object.isRequired, // eslint-disable-line react/forbid-prop-types css: PropTypes.func.isRequired, - _extendStyleFn: PropTypes.arrayOf(PropTypes.func), }; -export const withExtendStyles = withExtendStylesHoc; const EMPTY_STYLES = {}; const EMPTY_STYLES_FN = () => EMPTY_STYLES; @@ -47,7 +46,6 @@ export function withStyles( stylesPropName = 'styles', themePropName = 'theme', cssPropName = 'css', - extendStyleFnPropName = '_extendStyleFn', extendableStyles = {}, flushBefore = false, pureComponent = false, @@ -71,7 +69,7 @@ export function withStyles( : currentThemeRTL; } - function getStyleDef(direction, wrappedComponentName, extendStyleFns) { + function getStyleDef(direction, wrappedComponentName) { const currentTheme = getCurrentTheme(direction); let styleDef = direction === DIRECTIONS.LTR ? styleDefLTR @@ -98,14 +96,14 @@ export function withStyles( if (isRTL) { styleDefRTL = styleFn - ? ThemedStyleSheet.createRTL(styleFn, extendStyleFns, extendableStyles) + ? ThemedStyleSheet.createRTL(styleFn) : EMPTY_STYLES_FN; currentThemeRTL = registeredTheme; styleDef = styleDefRTL; } else { styleDefLTR = styleFn - ? ThemedStyleSheet.createLTR(styleFn, extendStyleFns, extendableStyles) + ? ThemedStyleSheet.createLTR(styleFn) : EMPTY_STYLES_FN; currentThemeLTR = registeredTheme; @@ -129,10 +127,10 @@ export function withStyles( return styleDef; } - function getState(direction, wrappedComponentName, extendStyleFns) { + function getState(direction, wrappedComponentName) { return { resolveMethod: getResolveMethod(direction), - styleDef: getStyleDef(direction, wrappedComponentName, extendStyleFns), + styleDef: getStyleDef(direction, wrappedComponentName), }; } @@ -151,16 +149,14 @@ export function withStyles( ? this.context[CHANNEL].getState() : defaultDirection; - this.extendStyleFns = props[extendStyleFnPropName] || []; - - this.state = getState(direction, wrappedComponentName, this.extendStyleFns); + this.state = getState(direction, wrappedComponentName); } componentDidMount() { if (this.context[CHANNEL]) { // subscribe to future direction changes this.channelUnsubscribe = this.context[CHANNEL].subscribe((direction) => { - this.setState(getState(direction, wrappedComponentName, this.extendStyleFns)); + this.setState(getState(direction, wrappedComponentName)); }); } } @@ -187,14 +183,10 @@ export function withStyles( resolveMethod, styleDef, } = this.state; - const { - [extendStyleFnPropName]: extendStyleFns, - ...rest - } = this.props; return ( withStyles( + extendStyles(styleFn, extendStyleFn, extendableStyles), + { + stylesPropName, + themePropName, + cssPropName, + extendableStyles, + flushBefore, + pureComponent, + }, + )(WrappedComponent); + } + return hoistNonReactStatics(WithStyles, WrappedComponent); }; } diff --git a/test/withExtendStyles_test.jsx b/test/withExtendStyles_test.jsx deleted file mode 100644 index 7d8010b9..00000000 --- a/test/withExtendStyles_test.jsx +++ /dev/null @@ -1,439 +0,0 @@ -import React from 'react'; -import { expect } from 'chai'; -import { render } from 'enzyme'; -import sinon from 'sinon-sandbox'; -import DirectionProvider, { DIRECTIONS } from 'react-with-direction/dist/DirectionProvider'; - -import ThemedStyleSheet from '../src/ThemedStyleSheet'; -import withExtendStyles from '../src/withExtendStyles'; -import { withStyles } from '../src/withStyles'; - -describe('withExtendStyles()', () => { - const defaultTheme = { - color: { - red: '#990000', - blue: '#334CFF', - }, - }; - - let testInterface; - - beforeEach(() => { - testInterface = { - create() {}, - createLTR() {}, - createRTL() {}, - resolve() {}, - resolveLTR() {}, - resolveRTL() {}, - flush: sinon.spy(), - }; - sinon.stub(testInterface, 'create').callsFake(styleHash => styleHash); - sinon.stub(testInterface, 'createLTR').callsFake(styleHash => styleHash); - sinon.stub(testInterface, 'createRTL').callsFake(styleHash => styleHash); - const fakeResolveMethod = styles => ({ - style: styles.reduce((result, style) => Object.assign(result, style)), - }); - sinon.stub(testInterface, 'resolve').callsFake(fakeResolveMethod); - sinon.stub(testInterface, 'resolveLTR') - .callsFake(fakeResolveMethod); - sinon.stub(testInterface, 'resolveRTL') - .callsFake(fakeResolveMethod); - - ThemedStyleSheet.registerTheme(defaultTheme); - ThemedStyleSheet.registerInterface(testInterface); - }); - - afterEach(() => { - sinon.restore(); - }); - - it('creates the styles in a non-directional context', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles(() => ({}))(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); - render(); - - expect(testInterface.createLTR.callCount).to.equal(1); - }); - - it('creates the styles in an LTR context', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles(() => ({}))(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); - render(( - - - - )); - expect(testInterface.createLTR.callCount).to.equal(1); - }); - - it('creates the styles in an RTL context', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles(() => ({}))(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); - render(( - - - - )); - expect(testInterface.createRTL.callCount).to.equal(1); - }); - - it('extends base styles', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - }, - }), - { - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ - container: { - background: 'green', - }, - }))(WrappedComponent); - render( - , - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'green', - color: 'blue', - }, - }); - }); - - it('extends base styles (multiple classes)', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - }, - innerContainer: { - background: 'white', - border: '1px solid black', - }, - content: { - fontSize: '25px', - }, - }), - { - extendableStyles: { - container: { - background: true, - }, - innerContainer: { - border: true, - }, - content: { - fontSize: true, - }, - }, - }, - )(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ - container: { - background: 'green', - }, - innerContainer: { - border: '10px solid green', - }, - content: { - fontSize: '12px', - }, - }))(WrappedComponent); - render( - , - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'green', - color: 'blue', - }, - innerContainer: { - background: 'white', - border: '10px solid green', - }, - content: { - fontSize: '12px', - }, - }); - }); - - it('extends base styles with multiple layers', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - fontSize: '10px', - }, - }), - { - extendableStyles: { - container: { - background: true, - fontSize: true, - }, - }, - }, - )(MyComponent); - - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ - container: { - background: 'green', - fontSize: '12px', - }, - }))(WrappedComponent); - - const WrappedComponentWithNestedExtendedStyles = withExtendStyles(() => ({ - container: { - fontSize: '20px', - }, - }))(WrappedComponentWithExtendedStyles); - - render( - , - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'green', - color: 'blue', - fontSize: '20px', - }, - }); - }); - - it('throws an error if an invalid extending style is provided', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - fontSize: '10px', - }, - }), - { - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ - container: { - // fontSize is invalid - fontSize: '12px', - }, - }))(WrappedComponent); - - expect(() => render( - , - )).to.throw(); - }); - - it('throws an error if extendableStyles is not defined, and an extending style is provided', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - fontSize: '10px', - }, - }), - { - // no extendableStyles - }, - )(MyComponent); - - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({ - container: { - // fontSize is invalid - fontSize: '12px', - }, - }))(WrappedComponent); - - expect(() => render( - , - )).to.throw(); - }); - - it('uses original base styles if no extending styles are provided', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles(() => ({ - container: { - background: 'red', - color: 'blue', - }, - }))(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); - render( - , - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'red', - color: 'blue', - }, - }); - }); - - it('receives the registered theme in the extend style function', (done) => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles(() => ({}))(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles((theme) => { - expect(theme).to.equal(defaultTheme); - done(); - return {}; - })(WrappedComponent); - render( - , - ); - }); - - it('allows the extend styles prop name to be customized', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - }, - }), - { - extendStyleFnPropName: 'foobar', - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles( - () => ({ - container: { - background: 'pink', - }, - }), - { extendStyleFnPropName: 'foobar' }, - )(WrappedComponent); - render( - , - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'pink', - color: 'blue', - }, - }); - }); - - it('passes the processed styles to the wrapped component', () => { - function MyComponent({ styles }) { - expect(styles).to.eql({ foo: { color: '#334CFF' } }); - return null; - } - - const WrappedComponent = withStyles( - ({ color }) => ({ - foo: { - color: color.red, - }, - }), - { - extendableStyles: { - foo: { - color: true, - }, - }, - }, - )(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(({ color }) => ({ - foo: { - color: color.blue, - }, - }))(WrappedComponent); - render(); - }); - - it('has a wrapped displayName', () => { - function MyComponent() { - return null; - } - - const result = withExtendStyles(() => ({}))(MyComponent); - expect(result.displayName).to.equal('withExtendStyles(MyComponent)'); - }); - - it('hoists statics', () => { - function MyComponent() { - return null; - } - MyComponent.foo = 'bar'; - - const WrappedComponent = withStyles(() => ({}))(MyComponent); - const WrappedComponentWithExtendedStyles = withExtendStyles(() => ({}))(WrappedComponent); - expect(WrappedComponentWithExtendedStyles.foo).to.equal('bar'); - }); -}); diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index e38b7e7e..5404d0f7 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -200,227 +200,6 @@ describe('withStyles()', () => { }); }); - describe('extendStyleFn', () => { - it('extends styles in a non-directional context', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - }, - }), - { - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - render( - ({ - container: { - background: 'green', - }, - }), - ]} - />, - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'green', - color: 'blue', - }, - }); - }); - - it('extends styles in an LTR context', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - }, - }), - { - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - render( - - ({ - container: { - background: 'green', - }, - }), - ]} - /> - , - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'green', - color: 'blue', - }, - }); - }); - - it('extends styles in an RTL context', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - }, - }), - { - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - render( - - ({ - container: { - background: 'green', - }, - }), - ]} - /> - , - ); - - expect(testInterface.createRTL.callCount).to.equal(1); - expect(testInterface.createRTL.getCall(0).args[0]).to.eql({ - container: { - background: 'green', - color: 'blue', - }, - }); - }); - - it('throws an error if an invalid style is extending', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({ - container: { - background: 'red', - color: 'blue', - }, - }), - { - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - - expect(() => render( - - ({ - container: { - // color is invalid - color: 'green', - }, - }), - ]} - /> - , - )).to.throw(); - }); - - it('receives the registered theme in the extend style function', (done) => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles(() => ({}))(MyComponent); - shallow( - { - expect(theme).to.equal(defaultTheme); - done(); - return {}; - }, - ]} - />, - ); - }); - - it('allows the extendStyleFn prop name to be customized', () => { - function MyComponent() { - return null; - } - - const WrappedComponent = withStyles( - () => ({}), - { - extendStyleFnPropName: 'newExtendStyleFn', - extendableStyles: { - container: { - background: true, - }, - }, - }, - )(MyComponent); - shallow( - ({ - container: { - background: 'green', - }, - }), - ]} - />, - ); - - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'green', - }, - }); - }); - }); - it('has a wrapped displayName', () => { function MyComponent() { return null; @@ -561,12 +340,16 @@ describe('withStyles()', () => { }))(MyComponent); // copied - expect(Wrapped.propTypes).to.include.keys('foo', '_extendStyleFn'); - expect(MyComponent.propTypes).to.include.keys('styles', 'theme', 'css'); + const expectedPropTypes = { ...MyComponent.propTypes }; + delete expectedPropTypes.styles; + delete expectedPropTypes.theme; + delete expectedPropTypes.css; + expect(Wrapped.propTypes).to.eql(expectedPropTypes); + expect(MyComponent.propTypes).to.include.keys('styles', 'theme'); expect(Wrapped.defaultProps).to.eql(MyComponent.defaultProps); - // // cloned + // cloned expect(Wrapped.propTypes).not.to.equal(MyComponent.propTypes); expect(Wrapped.defaultProps).not.to.equal(MyComponent.defaultProps); }); @@ -647,6 +430,227 @@ describe('withStyles()', () => { expect(testInterfaceResolveRTLStub.callCount).to.equal(1); }); }); + + describe('extendStyles', () => { + it('extends styles in a non-directional context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'green', + }, + })); + + render( + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + }); + }); + + it('extends styles in an LTR context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'green', + }, + })); + + render( + + + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + }); + }); + + it('extends styles in an RTL context', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'green', + }, + })); + render( + + + , + ); + + expect(testInterface.createRTL.callCount).to.equal(1); + expect(testInterface.createRTL.getCall(0).args[0]).to.eql({ + container: { + background: 'green', + color: 'blue', + }, + }); + }); + + it('throws an error if an invalid style is extending', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: true, + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + // color is invalid + color: 'green', + }, + })); + + expect(() => render( + + + , + )).to.throw(); + }); + + it('receives the registered theme in the extend style function', (done) => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles((theme) => { + expect(theme).to.equal(defaultTheme); + done(); + return {}; + }); + shallow( + , + ); + }); + + it('allows nested extends styles', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + fontSize: 12, + }, + }), + { + extendableStyles: { + container: { + background: true, + color: true, + fontSize: true, + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'green', + color: 'purple', + }, + })); + const NestedExtendedComponent = ExtendedComponent.extendStyles(() => ({ + container: { + background: 'pink', + }, + })); + + render( + , + ); + + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'pink', + color: 'purple', + fontSize: 12, + }, + }); + }); + }); }); describe('fallbacks', () => { From 9a4c5fc3a72cfc822b2f4d7bd86852f838b08ab4 Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Thu, 21 Mar 2019 19:33:35 -0700 Subject: [PATCH 04/16] Only validate in dev --- src/extendStyles.jsx | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index a22be7dc..5a0cc2b8 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -3,24 +3,23 @@ import deepmerge from 'deepmerge'; function validateStyle(style, extendableStyles, currentPath = '') { - // TODO: Only if DEV - if (style === null || Array.isArray(style) || typeof style !== 'object') { - return; - } - - const styleKeys = Object.keys(style); - if (styleKeys.length) { - styleKeys.forEach((styleKey) => { - const path = `${currentPath}.${styleKey}`; - const isValid = extendableStyles[styleKey]; - if (!isValid) { - throw new Error( - `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` - + 'the component\'s "extendableStyles" option.', - ); + if (process.env.NODE_ENV !== 'production') { + if (style === null || Array.isArray(style) || typeof style !== 'object') { + const styleKeys = Object.keys(style); + if (styleKeys.length) { + styleKeys.forEach((styleKey) => { + const path = `${currentPath}.${styleKey}`; + const isValid = extendableStyles[styleKey]; + if (!isValid) { + throw new Error( + `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` + + 'the component\'s "extendableStyles" option.', + ); + } + validateStyle(style[styleKey], extendableStyles[styleKey], path); + }); } - validateStyle(style[styleKey], extendableStyles[styleKey], path); - }); + } } } From 3785a375d6935e1ffc78276a4d261a15014736c9 Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Thu, 21 Mar 2019 22:16:20 -0700 Subject: [PATCH 05/16] Fix specs --- src/extendStyles.jsx | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index 5a0cc2b8..ee527294 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -5,20 +5,22 @@ import deepmerge from 'deepmerge'; function validateStyle(style, extendableStyles, currentPath = '') { if (process.env.NODE_ENV !== 'production') { if (style === null || Array.isArray(style) || typeof style !== 'object') { - const styleKeys = Object.keys(style); - if (styleKeys.length) { - styleKeys.forEach((styleKey) => { - const path = `${currentPath}.${styleKey}`; - const isValid = extendableStyles[styleKey]; - if (!isValid) { - throw new Error( - `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` + return; + } + + const styleKeys = Object.keys(style); + if (styleKeys.length) { + styleKeys.forEach((styleKey) => { + const path = `${currentPath}.${styleKey}`; + const isValid = extendableStyles[styleKey]; + if (!isValid) { + throw new Error( + `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` + 'the component\'s "extendableStyles" option.', - ); - } - validateStyle(style[styleKey], extendableStyles[styleKey], path); - }); - } + ); + } + validateStyle(style[styleKey], extendableStyles[styleKey], path); + }); } } } From 1a3df21124c4835283289cc5e2cf1669c76684fb Mon Sep 17 00:00:00 2001 From: Jordan Harband Date: Thu, 21 Mar 2019 23:51:38 -0700 Subject: [PATCH 06/16] Fix change Co-Authored-By: TaeKimJR --- src/withStyles.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/withStyles.jsx b/src/withStyles.jsx index ef50c5e5..f9a0f601 100644 --- a/src/withStyles.jsx +++ b/src/withStyles.jsx @@ -10,7 +10,6 @@ import brcastShape from 'react-with-direction/dist/proptypes/brcast'; import extendStyles from './extendStyles'; import ThemedStyleSheet from './ThemedStyleSheet'; - // Add some named exports to assist in upgrading and for convenience export const css = ThemedStyleSheet.resolveLTR; export const withStylesPropTypes = { From 4c3cff4c41aa7b04da56d2352403f69980298846 Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Fri, 22 Mar 2019 00:06:39 -0700 Subject: [PATCH 07/16] Cleanup validateStyle --- src/extendStyles.jsx | 13 +++--- test/withStyles_test.jsx | 86 ++++++++++++++++++++-------------------- 2 files changed, 51 insertions(+), 48 deletions(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index ee527294..72952911 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -2,24 +2,27 @@ import deepmerge from 'deepmerge'; -function validateStyle(style, extendableStyles, currentPath = '') { +// Recursively iterate through the style object, validating the same path exists in the +// extendableStyles object. +function validateStyle(style, extendableStyles, path = '') { if (process.env.NODE_ENV !== 'production') { - if (style === null || Array.isArray(style) || typeof style !== 'object') { + // Stop recursively validating when we hit a style's value + if (typeof style === 'string' || typeof style === 'number') { return; } const styleKeys = Object.keys(style); if (styleKeys.length) { styleKeys.forEach((styleKey) => { - const path = `${currentPath}.${styleKey}`; + const currentPath = `${path}.${styleKey}`; const isValid = extendableStyles[styleKey]; if (!isValid) { throw new Error( - `withStyles() extending style is invalid: ${path}. If this style is expected, add it to` + `withStyles() extending style is invalid: ${currentPath}. If this style is expected, add it to` + 'the component\'s "extendableStyles" option.', ); } - validateStyle(style[styleKey], extendableStyles[styleKey], path); + validateStyle(style[styleKey], extendableStyles[styleKey], currentPath); }); } } diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index 5404d0f7..dbf413c6 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -552,7 +552,7 @@ describe('withStyles()', () => { }); }); - it('throws an error if an invalid style is extending', () => { + it('allows nested extends styles', () => { function MyComponent() { return null; } @@ -562,47 +562,46 @@ describe('withStyles()', () => { container: { background: 'red', color: 'blue', + fontSize: 20, }, }), { extendableStyles: { container: { background: true, + color: true, + fontSize: true, }, }, }, )(MyComponent); const ExtendedComponent = WrappedComponent.extendStyles(() => ({ container: { - // color is invalid - color: 'green', + background: 'green', + fontSize: 12, + }, + })); + const NestedExtendedComponent = ExtendedComponent.extendStyles(() => ({ + container: { + background: 'pink', }, })); - expect(() => render( - - - , - )).to.throw(); - }); - - it('receives the registered theme in the extend style function', (done) => { - function MyComponent() { - return null; - } + render( + , + ); - const WrappedComponent = withStyles(() => ({}))(MyComponent); - const ExtendedComponent = WrappedComponent.extendStyles((theme) => { - expect(theme).to.equal(defaultTheme); - done(); - return {}; + expect(testInterface.createLTR.callCount).to.equal(1); + expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ + container: { + background: 'pink', + color: 'blue', + fontSize: 12, + }, }); - shallow( - , - ); }); - it('allows nested extends styles', () => { + it('throws an error if an invalid style is extending', () => { function MyComponent() { return null; } @@ -612,43 +611,44 @@ describe('withStyles()', () => { container: { background: 'red', color: 'blue', - fontSize: 12, }, }), { extendableStyles: { container: { background: true, - color: true, - fontSize: true, }, }, }, )(MyComponent); const ExtendedComponent = WrappedComponent.extendStyles(() => ({ container: { - background: 'green', - color: 'purple', - }, - })); - const NestedExtendedComponent = ExtendedComponent.extendStyles(() => ({ - container: { - background: 'pink', + // color is invalid + color: 'green', }, })); - render( - , - ); + expect(() => render( + + + , + )).to.throw(); + }); - expect(testInterface.createLTR.callCount).to.equal(1); - expect(testInterface.createLTR.getCall(0).args[0]).to.eql({ - container: { - background: 'pink', - color: 'purple', - fontSize: 12, - }, + it('receives the registered theme in the extend style function', (done) => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles((theme) => { + expect(theme).to.equal(defaultTheme); + done(); + return {}; }); + shallow( + , + ); }); }); }); From ca5b83af16c3f14bcbcdac4dabea1714b0bfbaef Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Sun, 24 Mar 2019 10:57:00 -0700 Subject: [PATCH 08/16] Collapse lines --- src/extendStyles.jsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index 72952911..5fad9ef1 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -18,8 +18,7 @@ function validateStyle(style, extendableStyles, path = '') { const isValid = extendableStyles[styleKey]; if (!isValid) { throw new Error( - `withStyles() extending style is invalid: ${currentPath}. If this style is expected, add it to` - + 'the component\'s "extendableStyles" option.', + `withStyles() extending style is invalid: ${currentPath}. If this style is expected, add it to the component's "extendableStyles" option.`, ); } validateStyle(style[styleKey], extendableStyles[styleKey], currentPath); From c48e84f757f226961ffbf89c533336a35bb65eaf Mon Sep 17 00:00:00 2001 From: Jordan Harband Date: Sun, 24 Mar 2019 11:15:54 -0700 Subject: [PATCH 09/16] Explicit check on length Co-Authored-By: TaeKimJR --- src/extendStyles.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index 72952911..16ff0723 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -12,7 +12,7 @@ function validateStyle(style, extendableStyles, path = '') { } const styleKeys = Object.keys(style); - if (styleKeys.length) { + if (styleKeys.length > 0) { styleKeys.forEach((styleKey) => { const currentPath = `${path}.${styleKey}`; const isValid = extendableStyles[styleKey]; From 065da866c1f6daacf1d9290387f49c6065ccdb0b Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Sun, 24 Mar 2019 11:16:19 -0700 Subject: [PATCH 10/16] Update base case for recursive validation --- src/extendStyles.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index 5fad9ef1..71698c74 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -7,7 +7,7 @@ import deepmerge from 'deepmerge'; function validateStyle(style, extendableStyles, path = '') { if (process.env.NODE_ENV !== 'production') { // Stop recursively validating when we hit a style's value - if (typeof style === 'string' || typeof style === 'number') { + if (!style || Array.isArray(style) || typeof style !== 'object') { return; } From 0dc71f44fbb958246f106f50bbd13504869c2901 Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Mon, 25 Mar 2019 15:14:36 -0700 Subject: [PATCH 11/16] Enforce predicate when defining extendableStyles --- src/extendStyles.jsx | 15 ++- test/withStyles_test.jsx | 282 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 275 insertions(+), 22 deletions(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index 8bc05517..c6de7540 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -6,8 +6,19 @@ import deepmerge from 'deepmerge'; // extendableStyles object. function validateStyle(style, extendableStyles, path = '') { if (process.env.NODE_ENV !== 'production') { - // Stop recursively validating when we hit a style's value + // Stop recursively validating when we hit a style's value and validate the value passes the + // style's predicate if (!style || Array.isArray(style) || typeof style !== 'object') { + const stylePredicate = extendableStyles; + if (typeof stylePredicate !== 'function') { + throw new Error(`withStyles() style predicate should be a function: "${path}". Check the component's "extendableStyles" option.`); + } + + const isValid = stylePredicate(style); + if (!isValid) { + throw new Error(`withStyles() style did not pass the predicate: "${path}": ${style}. Check the component's "extendableStyles" option.`); + } + return; } @@ -18,7 +29,7 @@ function validateStyle(style, extendableStyles, path = '') { const isValid = extendableStyles[styleKey]; if (!isValid) { throw new Error( - `withStyles() extending style is invalid: ${currentPath}. If this style is expected, add it to the component's "extendableStyles" option.`, + `withStyles() extending style is invalid: "${currentPath}". If this style is expected, add it to the component's "extendableStyles" option.`, ); } validateStyle(style[styleKey], extendableStyles[styleKey], currentPath); diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index dbf413c6..fe61fc95 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -447,7 +447,7 @@ describe('withStyles()', () => { { extendableStyles: { container: { - background: true, + background: () => true, }, }, }, @@ -486,7 +486,7 @@ describe('withStyles()', () => { { extendableStyles: { container: { - background: true, + background: () => true, }, }, }, @@ -527,7 +527,7 @@ describe('withStyles()', () => { { extendableStyles: { container: { - background: true, + background: () => true, }, }, }, @@ -568,9 +568,9 @@ describe('withStyles()', () => { { extendableStyles: { container: { - background: true, - color: true, - fontSize: true, + background: () => true, + color: () => true, + fontSize: () => true, }, }, }, @@ -601,7 +601,23 @@ describe('withStyles()', () => { }); }); - it('throws an error if an invalid style is extending', () => { + it('receives the registered theme in the extend style function', (done) => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles(() => ({}))(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles((theme) => { + expect(theme).to.equal(defaultTheme); + done(); + return {}; + }); + shallow( + , + ); + }); + + it('validates that the extending styles are defined in the extendableStyles option', () => { function MyComponent() { return null; } @@ -616,7 +632,7 @@ describe('withStyles()', () => { { extendableStyles: { container: { - background: true, + background: () => true, }, }, }, @@ -629,26 +645,252 @@ describe('withStyles()', () => { })); expect(() => render( - - - , + , )).to.throw(); }); - it('receives the registered theme in the extend style function', (done) => { + it('validates that the extended classNames are defined in the extendableStyles option', () => { function MyComponent() { return null; } - const WrappedComponent = withStyles(() => ({}))(MyComponent); - const ExtendedComponent = WrappedComponent.extendStyles((theme) => { - expect(theme).to.equal(defaultTheme); - done(); - return {}; - }); - shallow( + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: () => true, + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + innerContainer: { // not an extendable className + color: 'green', + }, + })); + + expect(() => render( , - ); + )).to.throw(); + }); + + + it('validates the extending style against the defined predicate (fail)', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: v => (v === 'red'), // predicate + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'green', // fails + }, + })); + + expect(() => render( + , + )).to.throw(); + }); + + it('validates the extending style against the defined predicate (pass)', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: v => (v === 'red'), // predicate + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'red', // passes + }, + })); + + expect(() => render( + , + )).to.not.throw(); + }); + + it('validates all extending styles against the defined predicates', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: v => (v === 'red'), + color: v => (v === 'green'), + fontSize: v => (v === 12), + }, + }, + }, + )(MyComponent); + const FailingColorComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'purple', // fails + color: 'green', + fontSize: 12, + }, + })); + const FailingBackgroundComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'red', + color: 15, // fails + fontSize: 12, + }, + })); + const FailingFontSizeComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'red', + color: 'green', + fontSize: '12', // fails + }, + })); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'red', + color: 'green', + fontSize: 12, + }, + })); + + expect(() => render( + , + )).to.throw(); + + expect(() => render( + , + )).to.throw(); + + expect(() => render( + , + )).to.throw(); + + expect(() => render( + , + )).to.not.throw(); + }); + + it('validates all extending styles across all classNames against the defined predicates', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + innerContainer: { + fontSize: 12, + }, + }), + { + extendableStyles: { + container: { + background: v => (v === 'red'), + color: v => (v === 'green'), + }, + innerContainer: { + fontSize: v => (v === 12), + }, + }, + }, + )(MyComponent); + const FailingColorComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'purple', // fails + color: 'green', + }, + innerContainer: { + fontSize: 12, + }, + })); + const FailingBackgroundComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'red', + color: 15, // fails + }, + innerContainer: { + fontSize: 12, + }, + })); + const FailingFontSizeComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'red', + color: 'green', + fontSize: '12', + }, + innerContainer: { + fontSize: '12', // fails + }, + })); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'red', + color: 'green', + }, + innerContainer: { + fontSize: 12, + }, + })); + + expect(() => render( + , + )).to.throw(); + + expect(() => render( + , + )).to.throw(); + + expect(() => render( + , + )).to.throw(); + + expect(() => render( + , + )).to.not.throw(); }); }); }); From 8a185d02a48e5d84aff541189598f5daca68db5c Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Mon, 25 Mar 2019 15:47:23 -0700 Subject: [PATCH 12/16] Give access to theme in predicates --- src/extendStyles.jsx | 8 ++-- test/withStyles_test.jsx | 90 +++++++++++++++++++++++++++++++++------- 2 files changed, 80 insertions(+), 18 deletions(-) diff --git a/src/extendStyles.jsx b/src/extendStyles.jsx index c6de7540..1f9abfd3 100644 --- a/src/extendStyles.jsx +++ b/src/extendStyles.jsx @@ -4,7 +4,7 @@ import deepmerge from 'deepmerge'; // Recursively iterate through the style object, validating the same path exists in the // extendableStyles object. -function validateStyle(style, extendableStyles, path = '') { +function validateStyle(style, extendableStyles, theme, path = '') { if (process.env.NODE_ENV !== 'production') { // Stop recursively validating when we hit a style's value and validate the value passes the // style's predicate @@ -14,7 +14,7 @@ function validateStyle(style, extendableStyles, path = '') { throw new Error(`withStyles() style predicate should be a function: "${path}". Check the component's "extendableStyles" option.`); } - const isValid = stylePredicate(style); + const isValid = stylePredicate(style, theme); if (!isValid) { throw new Error(`withStyles() style did not pass the predicate: "${path}": ${style}. Check the component's "extendableStyles" option.`); } @@ -32,7 +32,7 @@ function validateStyle(style, extendableStyles, path = '') { `withStyles() extending style is invalid: "${currentPath}". If this style is expected, add it to the component's "extendableStyles" option.`, ); } - validateStyle(style[styleKey], extendableStyles[styleKey], currentPath); + validateStyle(style[styleKey], extendableStyles[styleKey], theme, currentPath); }); } } @@ -47,7 +47,7 @@ export default function extendStyles( const baseStyle = baseStyleFn(theme); const extendingStyle = extendingStyleFn(theme); - validateStyle(extendingStyle, extendableStyles); + validateStyle(extendingStyle, extendableStyles, theme); const styles = deepmerge(baseStyle, extendingStyle); diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index fe61fc95..f0da29f4 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -743,6 +743,68 @@ describe('withStyles()', () => { )).to.not.throw(); }); + it('validates the extending style against the defined predicate with a theme (fail)', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: (v, t) => (v === t.color.red), // color = '#990000' + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + container: { + background: 'green', // fails + }, + })); + + expect(() => render( + , + )).to.throw(); + }); + + it('validates the extending style against the defined predicate with a theme (pass)', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + }), + { + extendableStyles: { + container: { + background: (v, t) => (v === t.color.red), // color = '#990000' + }, + }, + }, + )(MyComponent); + const ExtendedComponent = WrappedComponent.extendStyles(theme => ({ + container: { + background: theme.color.red, // pass + }, + })); + + expect(() => render( + , + )).to.not.throw(); + }); + it('validates all extending styles against the defined predicates', () => { function MyComponent() { return null; @@ -758,7 +820,7 @@ describe('withStyles()', () => { { extendableStyles: { container: { - background: v => (v === 'red'), + background: (v, t) => (v === t.color.red), color: v => (v === 'green'), fontSize: v => (v === 12), }, @@ -772,23 +834,23 @@ describe('withStyles()', () => { fontSize: 12, }, })); - const FailingBackgroundComponent = WrappedComponent.extendStyles(() => ({ + const FailingBackgroundComponent = WrappedComponent.extendStyles(theme => ({ container: { - background: 'red', + background: theme.color.red, color: 15, // fails fontSize: 12, }, })); - const FailingFontSizeComponent = WrappedComponent.extendStyles(() => ({ + const FailingFontSizeComponent = WrappedComponent.extendStyles(theme => ({ container: { - background: 'red', + background: theme.color.red, color: 'green', fontSize: '12', // fails }, })); - const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + const ExtendedComponent = WrappedComponent.extendStyles(theme => ({ container: { - background: 'red', + background: theme.color.red, color: 'green', fontSize: 12, }, @@ -829,7 +891,7 @@ describe('withStyles()', () => { { extendableStyles: { container: { - background: v => (v === 'red'), + background: (v, t) => (v === t.color.red), color: v => (v === 'green'), }, innerContainer: { @@ -847,18 +909,18 @@ describe('withStyles()', () => { fontSize: 12, }, })); - const FailingBackgroundComponent = WrappedComponent.extendStyles(() => ({ + const FailingBackgroundComponent = WrappedComponent.extendStyles(theme => ({ container: { - background: 'red', + background: theme.color.red, color: 15, // fails }, innerContainer: { fontSize: 12, }, })); - const FailingFontSizeComponent = WrappedComponent.extendStyles(() => ({ + const FailingFontSizeComponent = WrappedComponent.extendStyles(theme => ({ container: { - background: 'red', + background: theme.color.red, color: 'green', fontSize: '12', }, @@ -866,9 +928,9 @@ describe('withStyles()', () => { fontSize: '12', // fails }, })); - const ExtendedComponent = WrappedComponent.extendStyles(() => ({ + const ExtendedComponent = WrappedComponent.extendStyles(theme => ({ container: { - background: 'red', + background: theme.color.red, color: 'green', }, innerContainer: { From 80a9403bceaf651dde112fb02d8868e71a50918c Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Mon, 25 Mar 2019 23:23:25 -0700 Subject: [PATCH 13/16] Only add static property when defined --- src/withStyles.jsx | 2 +- test/withStyles_test.jsx | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/withStyles.jsx b/src/withStyles.jsx index f9a0f601..7e2f22ca 100644 --- a/src/withStyles.jsx +++ b/src/withStyles.jsx @@ -210,7 +210,7 @@ export function withStyles( WithStyles.defaultProps = { ...WrappedComponent.defaultProps }; } - if (extendableStyles) { + if (extendableStyles && Object.keys(extendableStyles).length !== 0) { WithStyles.extendStyles = extendStyleFn => withStyles( extendStyles(styleFn, extendStyleFn, extendableStyles), { diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index f0da29f4..987dbb88 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -954,6 +954,27 @@ describe('withStyles()', () => { , )).to.not.throw(); }); + + it('does not attach an "extendStyles" static property when "extendableStyles" option is not defined', () => { + function MyComponent() { + return null; + } + + const WrappedComponent = withStyles( + () => ({ + container: { + background: 'red', + color: 'blue', + }, + innerContainer: { + fontSize: 12, + }, + }), + {}, // extendableStyles is not defined + )(MyComponent); + + expect(WrappedComponent.extendStyles).to.equal(undefined); + }); }); }); From 97564c32d40c5f93987114e64c19d5680ae6855f Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Tue, 26 Mar 2019 00:21:18 -0700 Subject: [PATCH 14/16] README --- README.md | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/README.md b/README.md index 6648fb4a..1797f038 100644 --- a/README.md +++ b/README.md @@ -246,6 +246,38 @@ export default withStyles(({ color, unit }) => ({ Some components depend on previous styles to be ready in the component tree when mounting (e.g. dimension calculations). Some interfaces add styles to the page asynchronously, which is an obstacle for this. So, we provide the option of flushing the buffered styles before the rendering cycle begins. It is up to the interface to define what this means. +#### `extendableStyles` (default: `{}`) + +By default, the styles created using `withStyles()` will not be extendable. To extend a specific style or styles, you must define the paths and predicates that dictate which styles can be extended and with what values. This is useful if your component wants to define some constrained styles, while allowing consumers of the component to have flexibility around others. See the `extendStyles()` section below for more info on how to extend styles. + +```jsx +import React from 'react'; +import { css, withStyles } from './withStyles'; + +function MyComponent({ withStylesStyles }) { + return ( +
+ Try to be a rainbow in someone's cloud. +
+ ); +} + +export default withStyles( + ({ color, unit }) => ({ + container: { + color: color.primary, + marginBottom: 2 * unit, + }, + }), + { + extendableStyles: { + container: { + color: (value, theme) => true, + }, + }, + }, +)(MyComponent); +``` ## `css(...styles)` @@ -287,6 +319,58 @@ export default withStyles(({ color, unit }) => ({ `className` and `style` props must not be used on the same elements as `css()`. +## `extendStyles()` + +Components that define an "extendableStyles" option allow consumers to extend certain styles via the `extendStyles()` static property. This function takes in a styles thunk, exactly like `withStyles()`, and should return the extended styles. Any styles that are not explicitly defined in the "extendableStyles" option or do not pass the style's predicate function will throw an error. + +```jsx +import React from 'react'; +import { css, withStyles } from './withStyles'; + +function MyComponent({ withStylesStyles }) { + return ( +
+ Try to be a rainbow in someone's cloud. +
+ ); +} + +const BaseMyComponent = withStyles( + ({ color, unit }) => ({ + container: { + color: color.primary, + background: color.secondary, + marginBottom: 2 * unit, + }, + }), + { + extendableStyles: { + container: { + color: (value, theme) => true, // will allow all values of color + background: (value, { color }) => value === color.primary || color.secondary, // will only allow primary or secondary color + }, + }, + }, +)(MyComponent); + +const ExtendedMyComponent = BaseMyComponent.extendStyles(({ color }) => ({ + container: { + color: color.secondary, + background: color.primary, + }, +})); +``` + +You can extend a Component that already extending another Component. All validation will still occur. +```jsx +const NestedExtendedMyComponent = ExtendedMyComponent.extendStyles(() => ({ + container: { + color: 'red', + }, +})); +``` + + ## Examples ### With React Router's `Link` [React Router][react-router]'s [``][react-router-link] and [``][react-router-index-link] components accept `activeClassName='...'` and `activeStyle={{...}}` as props. As previously stated, `css(...styles)` must spread to JSX, so simply passing `styles.thing` or even `css(styles.thing)` directly will not work. In order to mimic `activeClassName`/`activeStyles` you can use React Router's [`withRouter()`][react-router-with-router] Higher Order Component to pass `router` as prop to your component and toggle styles based on [`router.isActive(pathOrLoc, indexOnly)`](react-router-is-active). This works because `` passes down the generated `className` from `css(..styles)` down through to the final leaf. From 3fd042738b42ddf6dc49a32d9644de4b1e43c50b Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Tue, 26 Mar 2019 00:26:16 -0700 Subject: [PATCH 15/16] Update README --- README.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 1797f038..916dc462 100644 --- a/README.md +++ b/README.md @@ -248,7 +248,7 @@ Some components depend on previous styles to be ready in the component tree when #### `extendableStyles` (default: `{}`) -By default, the styles created using `withStyles()` will not be extendable. To extend a specific style or styles, you must define the paths and predicates that dictate which styles can be extended and with what values. This is useful if your component wants to define some constrained styles, while allowing consumers of the component to have flexibility around others. See the `extendStyles()` section below for more info on how to extend styles. +By default, components created using `withStyles()` will not be extendable. To extend a component's style or styles, you must define the paths and predicates that dictate which styles can be extended and with what values. This is useful if your component wants to restrict some styles, while allowing consumers of the component to have flexibility around others. See the `extendStyles()` section for more info on how to extend styles. ```jsx import React from 'react'; @@ -266,6 +266,7 @@ export default withStyles( ({ color, unit }) => ({ container: { color: color.primary, + background: color.secondary, marginBottom: 2 * unit, }, }), @@ -273,12 +274,15 @@ export default withStyles( extendableStyles: { container: { color: (value, theme) => true, + background: (value, theme) => value === theme.color.primary || value === theme.color.secondary }, }, }, )(MyComponent); ``` +The `container.color` predicate allows for any value to be passed in. The `container.background` predicate only allows for the primary and secondary color to be passed. + ## `css(...styles)` This function takes styles that were processed by `withStyles()`, plain objects, or arrays of these things. It returns an object with an opaque structure that must be spread into a JSX element. @@ -346,8 +350,8 @@ const BaseMyComponent = withStyles( { extendableStyles: { container: { - color: (value, theme) => true, // will allow all values of color - background: (value, { color }) => value === color.primary || color.secondary, // will only allow primary or secondary color + color: (value, theme) => true, + background: (value, { color }) => value === color.primary || color.secondary, }, }, }, @@ -370,7 +374,6 @@ const NestedExtendedMyComponent = ExtendedMyComponent.extendStyles(() => ({ })); ``` - ## Examples ### With React Router's `Link` [React Router][react-router]'s [``][react-router-link] and [``][react-router-index-link] components accept `activeClassName='...'` and `activeStyle={{...}}` as props. As previously stated, `css(...styles)` must spread to JSX, so simply passing `styles.thing` or even `css(styles.thing)` directly will not work. In order to mimic `activeClassName`/`activeStyles` you can use React Router's [`withRouter()`][react-router-with-router] Higher Order Component to pass `router` as prop to your component and toggle styles based on [`router.isActive(pathOrLoc, indexOnly)`](react-router-is-active). This works because `` passes down the generated `className` from `css(..styles)` down through to the final leaf. From a364bd1e2bbecef4d6e8b9b4942cbbff4c84ca74 Mon Sep 17 00:00:00 2001 From: Tae Kim Date: Tue, 26 Mar 2019 09:32:01 -0700 Subject: [PATCH 16/16] Fix spec --- test/withStyles_test.jsx | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/test/withStyles_test.jsx b/test/withStyles_test.jsx index 987dbb88..c198454d 100644 --- a/test/withStyles_test.jsx +++ b/test/withStyles_test.jsx @@ -606,7 +606,18 @@ describe('withStyles()', () => { return null; } - const WrappedComponent = withStyles(() => ({}))(MyComponent); + const WrappedComponent = withStyles( + () => ({ + container: { + color: 'red', + }, + }), + { + extendableStyles: { + container: () => true, + }, + }, + )(MyComponent); const ExtendedComponent = WrappedComponent.extendStyles((theme) => { expect(theme).to.equal(defaultTheme); done();