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

Reduce lifecycle hooks and use of private methods #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 23 additions & 29 deletions src/injectSheet.js
Original file line number Diff line number Diff line change
@@ -1,62 +1,56 @@
import defaultInjectSheet from 'react-jss'

const managers = new WeakMap()

if (process.env.NODE_ENV === 'production') {
// eslint-disable-next-line no-console
console.error('react-jss-hmr should never be used in production!')
}

const managers = new WeakMap()
const deferredState = new WeakMap()

export default function injectSheet(...rest) {
const createHoc = defaultInjectSheet.apply(this, rest)
return (InnerComponent) => {
const Jss = createHoc(InnerComponent)

class HotJss extends Jss {
constructor(props, context) {
super(props, context)
deferredState.set(this.manager, this.state)
}

componentWillMount() {
// Note: this will never be called during hot module replacement, so we can
// use it as a place to store away the SheetManager
// This will never be called during hot module replacement, so we can use it as a place to
// store away the SheetManager
super.componentWillMount()
managers.set(Object.getPrototypeOf(this), this.manager)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this also be in the constructor? Or is the constructor when on HMR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No because the this in the constructor is not the proxy.
However I had a read of Redux’ connectAdvanced() that you referred to before and if I use their idea - a counter (version) - as the key then I could move this into the constructor.

I was thinking there wasn’t much reason to switch to the version idea because we need to track the SheetsManager anyway, but moving this code to the constructor might be a good enough reason. What do you think?

}

componentWillReceiveProps(nextProps, nextContext) {
super.componentWillReceiveProps(nextProps, nextContext)

const prevManager = managers.get(Object.getPrototypeOf(this))
const key = Object.getPrototypeOf(this)
const prevManager = managers.get(key)
const manager = this.manager
if (prevManager !== manager) {
// If the manager changed between mounting and updating, it was probably due to hot
// module replacement.
// In that case, we need to:
// 1. Call createState() again because although the constructor called it, RHL
// restored the instance’s previous state so now the classes in the SheetsManager
// do not match those in this.state
// 1. Restore the state created by the constructor, because RHL restored the instance’s
// previous state so now the classes in the SheetsManager do not match those in
// this.state
// 2. Call this.manage() so that the new SheetsManager will attach the new sheets
// 3. Call unmanage() on the old SheetsManager to clean up the old sheets
const {theme} = this.state
const newState = this.createState({theme}, nextProps)
this.manage(newState)
this.setState(newState)
// 3. Call unmanage() on the previous SheetsManager to clean up the old sheets
const prevTheme = this.state.theme
const nextState = deferredState.get(manager)
this.manage(nextState)
this.setState(nextState)
// the new sheets have been attached now, so we can detach the old ones
prevManager.unmanage(theme)
}
}

componentDidUpdate(prevProps, prevState) {
const key = Object.getPrototypeOf(this)
const prevManager = managers.get(key)
const manager = this.manager

const isHmrUpdate = prevManager && prevManager !== manager
if (isHmrUpdate) {
prevManager.unmanage(prevTheme)
// now start tracking the new manager, ready for next time
managers.set(key, manager)
}
if (!isHmrUpdate || prevState.dynamicSheet) {
// don't call when we don't have a previous dynamic sheet, otherwise it will throw if we
// added a new dynamic sheet via hmr
super.componentDidUpdate(prevProps, prevState)
}
deferredState.delete(manager)
}
}

Expand Down