Skip to content

Commit

Permalink
(split)warn consumers if themed styles are misused. fix cssinjs#1005 (c…
Browse files Browse the repository at this point in the history
…ssinjs#1006)

* warn consumers if themed styles are misused. fix cssinjs#1005

* Fix tests on node 10

upgrade karma-browserstack-launcher to at least 1.4.0,
because "internalBinding is not defined" due to old "natives" module
See more:
* karma-runner/karma-browserstack-launcher#140
* popcodeorg/popcode#1626

* add tests

* update snapshots

* add test for themed styles misuse warning not shown in prod

* Add dev expression babel plugin while testing

* Remove test and babel plugin from test setup

* Update size-snapshot
  • Loading branch information
iamstarkov authored and Henri committed Feb 3, 2019
1 parent 773b61e commit 129759b
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 18 deletions.
30 changes: 15 additions & 15 deletions .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 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 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 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)
})
})
})

0 comments on commit 129759b

Please sign in to comment.