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

[POC] Styled render callback component and refactored withStyles #9503

Closed
wants to merge 21 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1362466
switch prettier config to yaml
rosskevin Dec 15, 2017
a36895b
prototype - Styled render callback and withStyles refactoring
rosskevin Dec 15, 2017
f045ce6
fix getClasses helper to mount and obtain the styleSheet via sheetsRe…
rosskevin Dec 15, 2017
73fbad0
bad prop
rosskevin Dec 15, 2017
bf74fdd
rename dummy field class name to prevent confusion
rosskevin Dec 15, 2017
a56befa
I’m wrong about the props being modified - this proves it.
rosskevin Dec 15, 2017
0fe9008
hack showing difference causing sheet growth
rosskevin Dec 15, 2017
8be0604
proof that getStylesCreator creates a different result with same styl…
rosskevin Dec 15, 2017
52e2603
oops, NOW this proves it. sneaky.
rosskevin Dec 15, 2017
bb911be
add broken styles function equality test
rosskevin Dec 15, 2017
f42d156
fixed creator as key problem
rosskevin Dec 15, 2017
c042d5c
move the indexCounter into getStylesCreator - more relevant there.
rosskevin Dec 15, 2017
caece9a
fix passing of classes prop
rosskevin Dec 15, 2017
926db46
reestablish innerRef on withStyles
rosskevin Dec 15, 2017
8ff4562
ensure different creators for different style objects - just being pa…
rosskevin Dec 15, 2017
e4934e5
tests checkpoint
rosskevin Dec 15, 2017
7abdf6d
fix resolution of listenToTheme
rosskevin Dec 15, 2017
7717eba
Add tests for withStyles (withTheme) and fix Styled
rosskevin Dec 16, 2017
7060920
more fixes - first other spec green Paper.spec - should be reviewed b…
rosskevin Dec 16, 2017
ff5a53d
name should not be propagated
rosskevin Dec 16, 2017
a41f9e8
decouple props and establish render callback signature
rosskevin Dec 16, 2017
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
7 changes: 7 additions & 0 deletions .prettierrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
printWidth: 100
singleQuote: true
trailingComma: all
bracketSpacing: true
jsxBracketSameLine: false
parser: babylon
semi: true
11 changes: 0 additions & 11 deletions prettier.config.js

This file was deleted.

330 changes: 330 additions & 0 deletions src/styles/Styled.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,330 @@
import React from 'react';
import PropTypes from 'prop-types';
import warning from 'warning';
import getDisplayName from 'recompose/getDisplayName';
import contextTypes from 'react-jss/lib/contextTypes';
import { create } from 'jss';
import jssGlobal from 'jss-global';
import jssNested from 'jss-nested';
import jssCamelCase from 'jss-camel-case';
import jssDefaultUnit from 'jss-default-unit';
import jssVendorPrefixer from 'jss-vendor-prefixer';
import jssPropsSort from 'jss-props-sort';
import * as ns from 'react-jss/lib/ns';
import createMuiTheme from './createMuiTheme';
import themeListener from './themeListener';
import createGenerateClassName from './createGenerateClassName';
import getStylesCreator from './getStylesCreator';

export const preset = () => ({
plugins: [
jssGlobal(),
jssNested(),
jssCamelCase(),
jssDefaultUnit(),
jssVendorPrefixer(),
jssPropsSort(),
],
});

// New JSS instance.
const jss = create(preset());

// Use a singleton or the provided one by the context.
const generateClassName = createGenerateClassName();

// Global index counter to preserve source order.
// As we create the style sheet during componentWillMount lifecycle,
// children are handled after the parents, so the order of style elements would
// be parent->child. It is a problem though when a parent passes a className
// which needs to override any childs styles. StyleSheet of the child has a higher
// specificity, because of the source order.
// So our solution is to render sheets them in the reverse order child->sheet, so
// that parent has a higher specificity.
let indexCounter = Number.MIN_SAFE_INTEGER;

export const sheetsManager: Map<*, *> = new Map();

// We use the same empty object to ref count the styles that don't need a theme object.
const noopTheme = {};

// In order to have self-supporting components, we rely on default theme when not provided.
let defaultTheme;

function getDefaultTheme() {
if (defaultTheme) {
return defaultTheme;
}

defaultTheme = createMuiTheme();
return defaultTheme;
}

class Styled extends React.Component {
static defaultProps = {
Copy link
Member

Choose a reason for hiding this comment

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

I have been removing all the static properties as they get in the way of refactoring components from classes to functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't do that if it is still a class, it is invalid typescript and could harm our potential conversion. If it's a class, static props are the right way, the other way is just a hack after all.

Copy link
Member

@oliviertassinari oliviertassinari Dec 15, 2017

Choose a reason for hiding this comment

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

Oh, TypeScript doesn't support this?

Copy link
Member Author

Choose a reason for hiding this comment

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

You have to: augment the definition of the target, or cast it. So it supports it, but you have to cast it to the type first. In other words it is uglier on the outside, vs static class prop.

Copy link
Member

Choose a reason for hiding this comment

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

Alright. At least I can keep using this pattern at work. It's saving us time refactoring component class <-> function.

withTheme: false,
};
constructor(props, context) {
super(props, context);

const { muiThemeProviderOptions } = this.context;

this.jss = this.context[ns.jss] || jss;

if (muiThemeProviderOptions) {
if (muiThemeProviderOptions.sheetsManager) {
this.sheetsManager = muiThemeProviderOptions.sheetsManager;
}

this.disableStylesGeneration = muiThemeProviderOptions.disableStylesGeneration;
}

this.stylesCreator = getStylesCreator(props.styles);
if (this.stylesCreator.options.index === undefined) {
indexCounter += 1;
this.stylesCreator.options.index = indexCounter;
}

warning(
indexCounter < 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

< 0 ?

Copy link
Member

Choose a reason for hiding this comment

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

This warning isn't that useful. It' in case you called the component many many times. But would have probably found the issue before.

Copy link
Member

Choose a reason for hiding this comment

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

Still, the warning doesn't harm.

[
'Material-UI: you might have a memory leak.',
'The indexCounter is not supposed to grow that much.',
].join(' '),
);

// Attach the stylesCreator to the instance of the component as in the context
// of react-hot-loader the hooks can be executed in a different closure context:
// https://github.com/gaearon/react-hot-loader/blob/master/src/patch.dev.js#L107
this.stylesCreatorSaved = this.stylesCreator;
this.sheetOptions = {
generateClassName,
...this.context[ns.sheetOptions],
};
// We use || as it's lazy evaluated.
this.theme = props.listenToTheme
? themeListener.initial(context) || getDefaultTheme()
: noopTheme;
}

state = {};

componentWillMount() {
this.attach(this.theme);
}

componentDidMount() {
const { listenToTheme } = this.props;
if (!listenToTheme) {
return;
}

this.unsubscribeId = themeListener.subscribe(this.context, theme => {
const oldTheme = this.theme;
this.theme = theme;
this.attach(this.theme);

// Rerender the component so the underlying component gets the theme update.
// By theme update we mean receiving and applying the new class names.
this.setState({}, () => {
this.detach(oldTheme);
});
});
}

componentWillReceiveProps() {
// react-hot-loader specific logic
if (this.stylesCreatorSaved === this.stylesCreator || process.env.NODE_ENV === 'production') {
return;
}

this.detach(this.theme);
this.stylesCreatorSaved = this.stylesCreator;
this.attach(this.theme);
}

componentWillUnmount() {
this.detach(this.theme);

if (this.unsubscribeId !== null) {
themeListener.unsubscribe(this.context, this.unsubscribeId);
}
}

attach(theme) {
if (this.disableStylesGeneration) {
return;
}

const { Component, flip, options: styleSheetOptions } = this.props;
const stylesCreatorSaved = this.stylesCreatorSaved;
let sheetManager = this.sheetsManager.get(stylesCreatorSaved);

if (!sheetManager) {
sheetManager = new Map();
this.sheetsManager.set(stylesCreatorSaved, sheetManager);
}

let sheetManagerTheme = sheetManager.get(theme);

if (!sheetManagerTheme) {
sheetManagerTheme = {
refs: 0,
sheet: null,
};
sheetManager.set(theme, sheetManagerTheme);
}

if (sheetManagerTheme.refs === 0) {
const styles = stylesCreatorSaved.create(theme, styleSheetOptions.name);
let meta;

if (process.env.NODE_ENV !== 'production') {
// REVIEW this is going to be bogus - can't have every stylesheet used this way
// popping out as 'Styled'
meta = styleSheetOptions.name || (Component ? getDisplayName(Component) : 'Styled');
}

const sheet = this.jss.createStyleSheet(styles, {
meta,
flip: typeof flip === 'boolean' ? flip : theme.direction === 'rtl',
link: false,
...this.sheetOptions,
...stylesCreatorSaved.options,
name: styleSheetOptions.name,
...styleSheetOptions,
});

sheetManagerTheme.sheet = sheet;
sheet.attach();

const sheetsRegistry = this.context[ns.sheetsRegistry];
if (sheetsRegistry) {
sheetsRegistry.add(sheet);
}
}

sheetManagerTheme.refs += 1;
}

detach(theme) {
if (this.disableStylesGeneration) {
return;
}

const stylesCreatorSaved = this.stylesCreatorSaved;
const sheetManager = this.sheetsManager.get(stylesCreatorSaved);
const sheetManagerTheme = sheetManager.get(theme);

sheetManagerTheme.refs -= 1;

if (sheetManagerTheme.refs === 0) {
sheetManager.delete(theme);
this.jss.removeStyleSheet(sheetManagerTheme.sheet);
const sheetsRegistry = this.context[ns.sheetsRegistry];
if (sheetsRegistry) {
sheetsRegistry.remove(sheetManagerTheme.sheet);
}
}
}

unsubscribeId = null;
jss = null;
sheetsManager = sheetsManager;
disableStylesGeneration = false;
stylesCreator = null;
stylesCreatorSaved = null;
theme = null;
sheetOptions = null;
theme = null;

render() {
const { children, classes: classesProp, Component, styles, withTheme, ...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 ' +
`${Component ? getDisplayName(Component) : '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 more = {};

// Provide the theme to the wrapped component.
// So we don't have to use the `withTheme()` Higher-order Component.
if (withTheme) {
more.theme = this.theme;
}

return children({ ...classes, ...more, ...other });
}
}

Styled.propTypes = {
children: PropTypes.func,
/**
* Useful to extend the style applied to components.
*/
classes: PropTypes.object,
/**
* @ignore only for better messages when used with withStyles
*/
Component: PropTypes.func,
flip: PropTypes.bool,
listenToTheme: PropTypes.bool,
/**
* StyleSheet options
*/
options: PropTypes.object,
/**
* Styles object or function that accepts the Theme and returns an object.
*/
styles: PropTypes.oneOfType([PropTypes.object, PropTypes.func]).isRequired,
withTheme: PropTypes.bool,
};

Styled.contextTypes = {
muiThemeProviderOptions: PropTypes.object,
...contextTypes,
...themeListener.contextTypes, // REVIEW (listenToTheme ? themeListener.contextTypes : {}),
};

export default Styled;
Loading