Skip to content

Commit

Permalink
[withStyles] Memoization the classes property (#11202)
Browse files Browse the repository at this point in the history 
* memoization of classes in withStyles

* ready to ship
  • Loading branch information
@quisido @oliviertassinari
quisido authored and oliviertassinari committed May 4, 2018
1 parent 885e123 commit a537af5
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 57 deletions.
2 changes: 1 addition & 1 deletion .size-limit
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"name": "The size of all the modules of material-ui.",
"webpack": true,
"path": "packages/material-ui/build/index.js",
"limit": "101.3 KB"
"limit": "101.4 KB"
},
{
"name": "The main bundle of the docs",
Expand Down
122 changes: 74 additions & 48 deletions packages/material-ui/src/styles/withStyles.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,15 @@ const withStyles = (stylesOrCreator, options = {}) => Component => {
this.theme = listenToTheme ? themeListener.initial(context) || getDefaultTheme() : noopTheme;

this.attach(this.theme);

this.cacheClasses = {
// Cache for the finalized classes value.
value: null,
// Cache for the last used classes prop pointer.
lastProp: null,
// Cache for the last used rendered classes pointer.
lastJSS: {},
};
}

state = {};
Expand Down Expand Up @@ -135,6 +144,69 @@ const withStyles = (stylesOrCreator, options = {}) => Component => {
}
}

getClasses() {
// Tracks if either the rendered classes or classes prop has changed,
// requiring the generation of a new finalized classes object.
let generate = false;

if (!this.disableStylesGeneration) {
const sheetManager = this.sheetsManager.get(this.stylesCreatorSaved);
const sheetsManagerTheme = sheetManager.get(this.theme);
if (sheetsManagerTheme.sheet.classes !== this.cacheClasses.lastJSS) {
this.cacheClasses.lastJSS = sheetsManagerTheme.sheet.classes;
generate = true;
}
}

if (this.props.classes !== this.cacheClasses.lastProp) {
this.cacheClasses.lastProp = this.props.classes;
generate = true;
}

if (generate) {
if (this.props.classes) {
this.cacheClasses.value = {
...this.cacheClasses.lastJSS,
...Object.keys(this.props.classes).reduce((accumulator, key) => {
warning(
this.cacheClasses.lastJSS[key] || this.disableStylesGeneration,
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not implemented in ${getDisplayName(
Component,
)}.`,
`You can only override one of the following: ${Object.keys(
this.cacheClasses.lastJSS,
).join(',')}`,
].join('\n'),
);

warning(
!this.props.classes[key] || typeof this.props.classes[key] === 'string',
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not valid for ${getDisplayName(
Component,
)}.`,
`You need to provide a non empty string instead of: ${this.props.classes[key]}.`,
].join('\n'),
);

if (this.props.classes[key]) {
accumulator[key] = `${this.cacheClasses.lastJSS[key]} ${this.props.classes[key]}`;
}

return accumulator;
}, {}),
};
} else {
this.cacheClasses.value = this.cacheClasses.lastJSS;
}
}

return this.cacheClasses.value;
}

attach(theme) {
if (this.disableStylesGeneration) {
return;
Expand Down Expand Up @@ -219,53 +291,7 @@ const withStyles = (stylesOrCreator, options = {}) => Component => {
unsubscribeId = null;

render() {
const { classes: classesProp, innerRef, ...other } = this.props;

let classes;
let renderedClasses = {};

if (!this.disableStylesGeneration) {
const sheetManager = this.sheetsManager.get(this.stylesCreatorSaved);
const sheetsManagerTheme = sheetManager.get(this.theme);
renderedClasses = sheetsManagerTheme.sheet.classes;
}

if (classesProp) {
classes = {
...renderedClasses,
...Object.keys(classesProp).reduce((accumulator, key) => {
warning(
renderedClasses[key] || this.disableStylesGeneration,
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not implemented in ${getDisplayName(
Component,
)}.`,
`You can only override one of the following: ${Object.keys(renderedClasses).join(
',',
)}`,
].join('\n'),
);

warning(
!classesProp[key] || typeof classesProp[key] === 'string',
[
`Material-UI: the key \`${key}\` ` +
`provided to the classes property is not valid for ${getDisplayName(Component)}.`,
`You need to provide a non empty string instead of: ${classesProp[key]}.`,
].join('\n'),
);

if (classesProp[key]) {
accumulator[key] = `${renderedClasses[key]} ${classesProp[key]}`;
}

return accumulator;
}, {}),
};
} else {
classes = renderedClasses;
}
const { classes, innerRef, ...other } = this.props;

const more = getThemeProps({ theme: this.theme, name });

Expand All @@ -275,7 +301,7 @@ const withStyles = (stylesOrCreator, options = {}) => Component => {
more.theme = this.theme;
}

return <Component {...more} classes={classes} ref={innerRef} {...other} />;
return <Component {...more} classes={this.getClasses()} ref={innerRef} {...other} />;
}
}

Expand Down
45 changes: 37 additions & 8 deletions packages/material-ui/src/styles/withStyles.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('withStyles', () => {

it('should ignore undefined property', () => {
const wrapper = shallow(<StyledComponent1 classes={{ root: undefined }} />);
assert.deepEqual(wrapper.props().classes, { root: `${classes.root}` });
assert.deepEqual(wrapper.props().classes, { root: classes.root });
assert.strictEqual(consoleErrorMock.callCount(), 0);
});

Expand All @@ -90,21 +90,50 @@ describe('withStyles', () => {
/Material-UI: the key `root` provided to the classes property is not valid/,
);
});
});

it('should recycle the object between two render if possible', () => {
describe('prop: innerRef', () => {
it('should provide a ref on the inner component', () => {
const handleRef = spy();
mount(<StyledComponent1 innerRef={handleRef} />);
assert.strictEqual(handleRef.callCount, 1);
});
});

describe('cache', () => {
it('should recycle with no classes property', () => {
const wrapper = mount(<StyledComponent1 />);
const classes1 = wrapper.find(Empty).props().classes;
wrapper.update();
const classes2 = wrapper.find(Empty).props().classes;
assert.strictEqual(classes1, classes2);
});
});

describe('prop: innerRef', () => {
it('should provide a ref on the inner component', () => {
const handleRef = spy();
mount(<StyledComponent1 innerRef={handleRef} />);
assert.strictEqual(handleRef.callCount, 1);
it('should recycle even when a classes property is provided', () => {
const inputClasses = { root: 'foo' };
const wrapper = mount(<StyledComponent1 classes={inputClasses} />);
const classes1 = wrapper.find(Empty).props().classes;
wrapper.setProps({
classes: inputClasses,
});
const classes2 = wrapper.find(Empty).props().classes;
assert.strictEqual(classes1, classes2);
});

it('should invalidate the cache', () => {
const wrapper = mount(<StyledComponent1 classes={{ root: 'foo' }} />);
const classes1 = wrapper.find(Empty).props().classes;
assert.deepEqual(classes1, {
root: `${classes.root} foo`,
});
wrapper.setProps({
classes: { root: 'bar' },
});
const classes2 = wrapper.find(Empty).props().classes;
assert.notStrictEqual(classes1, classes2);
assert.deepEqual(classes2, {
root: `${classes.root} bar`,
});
});
});
});
Expand Down

0 comments on commit a537af5

Please sign in to comment.