Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

warn consumers if themed styles are misused. fix #1005 #1006

Merged
merged 12 commits into from
Feb 3, 2019
1 change: 1 addition & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- [jss] Simplify cloneStyle function ([#1003](https://github.com/cssinjs/jss/pull/1003))
- [internal] Improve publish script for creating github release ([#999](https://github.com/cssinjs/jss/pull/999))
- [react-jss] Warn about themed styles misuse ([#1006](https://github.com/cssinjs/jss/pull/1006))

### Bug fixes

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@
"karma": "^1.3.0",
"karma-benchmark": "^0.6.0",
"karma-benchmark-reporter": "^0.1.1",
"karma-browserstack-launcher": "^1.1.1",
"karma-browserstack-launcher": "^1.4.0",
"karma-chrome-launcher": "^2.0.0",
"karma-coverage": "^1.1.1",
"karma-mocha": "^1.3.0",
Expand Down
30 changes: 15 additions & 15 deletions packages/react-jss/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
{
"dist/react-jss.js": {
"bundled": 132019,
"minified": 44763,
"gzipped": 13242
"bundled": 132277,
"minified": 44968,
"gzipped": 13372
},
"dist/react-jss.min.js": {
"bundled": 100287,
"minified": 34922,
"gzipped": 10492
"bundled": 100312,
"minified": 34926,
"gzipped": 10494
},
"dist/react-jss.cjs.js": {
"bundled": 14410,
"minified": 6632,
"gzipped": 2198
"bundled": 14771,
"minified": 6929,
"gzipped": 2359
},
"dist/react-jss.esm.js": {
"bundled": 13749,
"minified": 6067,
"gzipped": 2085,
"bundled": 14090,
"minified": 6349,
"gzipped": 2244,
"treeshaked": {
"rollup": {
"code": 1925,
"import_statements": 436
"code": 1946,
"import_statements": 457
},
"webpack": {
"code": 3285
"code": 3340
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion packages/react-jss/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
"jss": "10.0.0-alpha.9",
"jss-preset-default": "10.0.0-alpha.9",
"prop-types": "^15.6.0",
"theming": "^3.0.3"
"theming": "^3.0.3",
"tiny-warning": "^1.0.2"
},
"devDependencies": {
"@types/react": "^16.7.20"
Expand Down
10 changes: 8 additions & 2 deletions packages/react-jss/src/withStyles.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React, {Component, type ComponentType, type Node} from 'react'
import hoistNonReactStatics from 'hoist-non-react-statics'
import {getDynamicStyles, SheetsManager, type StyleSheet} from 'jss'
import {ThemeContext} from 'theming'
import warning from 'tiny-warning'

import type {HOCProps, Options, Styles, InnerProps} from './types'
import getDisplayName from './getDisplayName'
Expand Down Expand Up @@ -35,10 +36,15 @@ let managersCounter = 0

const NoRenderer = (props: {children?: Node}) => props.children || null

const getStyles = <Theme: {}>(styles: Styles<Theme>, theme: Theme) => {
const getStyles = <Theme: {}>(styles: Styles<Theme>, theme: Theme, displayName: string) => {
if (typeof styles !== 'function') {
return styles
}
warning(
styles.length !== 0,
`[JSS] <${displayName} />'s styles function doesn't rely on a theme. We recommend to rewrite it to plain object. Read more: https://github.com/cssinjs/jss/blob/master/docs/react-jss.md#basic`
)

return styles(theme)
}

Expand Down Expand Up @@ -134,7 +140,7 @@ export default function withStyles<Theme: {}, S: Styles<Theme>>(
return staticSheet
}

const themedStyles = getStyles(styles, theme)
const themedStyles = getStyles(styles, theme, displayName)
const contextSheetOptions = this.props.jssContext.sheetOptions
staticSheet = this.jss.createStyleSheet(themedStyles, {
...sheetOptions,
Expand Down
36 changes: 36 additions & 0 deletions packages/react-jss/src/withStyles.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,4 +244,40 @@ describe('React-JSS: withStyles', () => {
process.env.NODE_ENV = 'development'
})
})

describe('.withStyles() properly warns about themed styles misuse', () => {
beforeEach(() => {
spy(console, 'warn')
})

afterEach(() => {
console.warn.restore()
})

it('warn if themed styles dont use theme', () => {
function DisplayNameTest() {
return null
}
const MyComponent = withStyles(() => ({}))(DisplayNameTest)

TestRenderer.create(<MyComponent />)

expect(
console.warn.calledWithExactly(
`Warning: [JSS] <DisplayNameTest />'s styles function doesn't rely on a theme. We recommend to rewrite it to plain object. Read more: https://github.com/cssinjs/jss/blob/master/docs/react-jss.md#basic`
)
).to.be(true)
})

it('doesnt warn if themed styles _do use_ theme', () => {
function DisplayNameTest() {
return null
}
const MyComponent = withStyles(theme => ({}))(DisplayNameTest) // eslint-disable-line no-unused-vars

TestRenderer.create(<MyComponent />)

expect(console.warn.called).to.be(false)
})
})
})
Loading