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.

Sign up for GitHub

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

[styles] Improve ref forwarding #13676

Merged
merged 3 commits into from
Mar 5, 2019
Merged

[styles] Improve ref forwarding #13676

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4998d69
[styles] Forward refs in with* hocs
eps1lon Mar 5, 2019
435f396
[styles] Comment the deprecation, we are still using innerRef
oliviertassinari Mar 5, 2019
ff974b3
fix eslint warnings
oliviertassinari Mar 5, 2019
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
19 changes: 8 additions & 11 deletions docs/src/modules/components/PageTitle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,15 @@ import PageContext from 'docs/src/modules/components/PageContext';

// TODO: it really wants to be named useTitle but we're not quite there yet.
function PageTitle(props) {
return (
<PageContext.Consumer>
{({ activePage }) => {
if (!activePage) {
throw new Error('Missing activePage.');
}
const { activePage } = React.useContext(PageContext);

const title = activePage.title !== false ? pageToTitle(activePage) : null;
return props.children(title);
}}
</PageContext.Consumer>
);
if (!activePage) {
throw new Error('Missing activePage.');
}

const title = activePage.title !== false ? pageToTitle(activePage) : null;

return props.children(title);
}

PageTitle.propTypes = {
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui-styles/src/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* istanbul ignore file */
Copy link
Member

Choose a reason for hiding this comment

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

I'm removing all the istanbul ignore, @eps1lon is right, let's leave the 100% utopia.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be fair that broken window analogy you linked made total sense to me. I just think that these comments won't help in that regard. At least this way we have the actual number to look at. Previously it was 100% (kind of; who knows how good it is actually). Now we know how good/bad we are and have tools helping us analyze it. Codesearch is not really that helpful IMO. It's like prefering // TODO over creating issues.

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

/* eslint-disable no-underscore-dangle */

import { ponyfillGlobal } from '@material-ui/utils';
Expand Down
8 changes: 4 additions & 4 deletions packages/material-ui-styles/src/withStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ const withStyles = (stylesOrCreator, options = {}) => Component => {
return null;
}

return new Error(
'Material-UI: The `innerRef` prop is deprecated and will be removed in v5. ' +
'Refs are now automatically forwarded to the inner component.',
);
// return new Error(
Copy link
Member

@oliviertassinari oliviertassinari Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It's raising a warning as the ButtonBase is still using this API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I think I annotated it with @deprecated somewhere to indicate a "soft deprecation". In a perfect world the eslint rule would detect these. I don't like runtime deprecation warnings that have the same level as any other warning (hello ReactDOM.findDOMNode). A lint rule makes more sense to me.

// 'Material-UI: The `innerRef` prop is deprecated and will be removed in v5. ' +
// 'Refs are now automatically forwarded to the inner component.',
// );
}),
};

Expand Down
60 changes: 30 additions & 30 deletions packages/material-ui-styles/src/withStyles.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ describe('withStyles', () => {

const ref = React.createRef();
mount(
<>
<React.Fragment>
Copy link
Member

@oliviertassinari oliviertassinari Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What should we do with the Fragment. Should we embrace <> everywhere, should only use React.Fragment or it doesn't matter 🤔. I'm reducing entropy until we decide.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think React.Fragment is required if fragment refs land. So maybe enforce that?

<StyledTarget ref={ref} />
</>,
</React.Fragment>,
);
assert.instanceOf(ref.current, TargetComponent);
});
Expand All @@ -66,39 +66,39 @@ describe('withStyles', () => {

const ref = React.createRef();
mount(
<>
<React.Fragment>
<StyledTarget ref={ref} />
</>,
</React.Fragment>,
);
assert.strictEqual(ref.current.nodeName, 'DIV');
});

describe('innerRef', () => {
beforeEach(() => {
consoleErrorMock.spy();
});

afterEach(() => {
consoleErrorMock.reset();
PropTypes.resetWarningCache();
});

it('is deprecated', () => {
const ThemedDiv = withStyles({})('div');

mount(
<>
<ThemedDiv innerRef={React.createRef()} />
</>,
);

assert.strictEqual(consoleErrorMock.callCount(), 1);
assert.include(
consoleErrorMock.args()[0][0],
'Warning: Failed prop type: Material-UI: The `innerRef` prop is deprecated',
);
});
});
// describe('innerRef', () => {
// beforeEach(() => {
// consoleErrorMock.spy();
// });

// afterEach(() => {
// consoleErrorMock.reset();
// PropTypes.resetWarningCache();
// });

// it('is deprecated', () => {
// const ThemedDiv = withStyles({})('div');

// mount(
// <React.Fragment>
// <ThemedDiv innerRef={React.createRef()} />
// </React.Fragment>,
// );

// assert.strictEqual(consoleErrorMock.callCount(), 1);
// assert.include(
// consoleErrorMock.args()[0][0],
// 'Warning: Failed prop type: Material-UI: The `innerRef` prop is deprecated',
// );
// });
// });
});

it('should forward the properties', () => {
Expand Down
12 changes: 6 additions & 6 deletions packages/material-ui-styles/src/withTheme.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ describe('withTheme', () => {

const ref = React.createRef();
mount(
<>
<React.Fragment>
<ThemedTarget ref={ref} />
</>,
</React.Fragment>,
);

assert.instanceOf(ref.current, TargetComponent);
Expand All @@ -81,9 +81,9 @@ describe('withTheme', () => {

const ref = React.createRef();
mount(
<>
<React.Fragment>
<ThemedTarget ref={ref} />
</>,
</React.Fragment>,
);

assert.strictEqual(ref.current.nodeName, 'DIV');
Expand All @@ -103,9 +103,9 @@ describe('withTheme', () => {
const ThemedDiv = withTheme('div');

mount(
<>
<React.Fragment>
<ThemedDiv innerRef={React.createRef()} />
</>,
</React.Fragment>,
);

assert.strictEqual(consoleErrorMock.callCount(), 1);
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui-utils/src/chainPropTypes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
function chainPropTypes(propType1, propType2) {
/* istanbul ignore if */
if (process.env.NODE_ENV === 'production') {
return () => null;
}
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui-utils/src/exactProp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
export const specialProperty = 'exact-prop: \u200b';

function exactProp(propTypes) {
/* istanbul ignore if */
if (process.env.NODE_ENV === 'production') {
return propTypes;
}
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/ButtonBase/createRippleHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ let createRippleHandler = (instance, eventName, action, cb) => event => {
return true;
};

/* istanbul ignore if */
if (typeof window === 'undefined') {
createRippleHandler = () => () => {};
}
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/CircularProgress/CircularProgress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,6 @@ CircularProgress.propTypes = {
* This only works if variant is `indeterminate`.
*/
disableShrink: chainPropTypes(PropTypes.bool, props => {
/* istanbul ignore if */
if (props.disableShrink && props.variant !== 'indeterminate') {
return new Error(
'Material-UI: you have provided the `disableShrink` property ' +
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/Divider/Divider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ Divider.propTypes = {
* Instead use `variant="inset"`.
*/
inset: chainPropTypes(PropTypes.bool, props => {
/* istanbul ignore if */
if (props.inset) {
return new Error(
'Material-UI: you are using the deprecated `inset` property ' +
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/ExpansionPanel/ExpansionPanel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ ExpansionPanel.propTypes = {
);
}

/* istanbul ignore if */
if (!React.isValidElement(summary)) {
return new Error(
`Material-UI: Expected the first child of ExpansionPanel to be a valid element.${
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/styles/MuiThemeProvider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ MuiThemeProviderOld.contextTypes = {
muiThemeProviderOptions: PropTypes.object,
};

/* istanbul ignore if */
if (!ponyfillGlobal.__MUI_STYLES__) {
ponyfillGlobal.__MUI_STYLES__ = {};
}
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/styles/withStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,6 @@ const withStylesOld = (stylesOrCreator, options = {}) => Component => {
return WithStyles;
};

/* istanbul ignore if */
if (!ponyfillGlobal.__MUI_STYLES__) {
ponyfillGlobal.__MUI_STYLES__ = {};
}
Expand Down
2 changes: 0 additions & 2 deletions packages/material-ui/src/styles/withTheme.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ function getDefaultTheme() {

// Provide the theme object as a property to the input component.
const withThemeOld = Component => {
/* istanbul ignore if */
if (process.env.NODE_ENV !== 'production' && Component === undefined) {
throw new Error(
[
Expand Down Expand Up @@ -80,7 +79,6 @@ const withThemeOld = Component => {
return WithTheme;
};

/* istanbul ignore if */
if (!ponyfillGlobal.__MUI_STYLES__) {
ponyfillGlobal.__MUI_STYLES__ = {};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/test-utils/testRef.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ function assertDOMNode(node) {
*/
export default function testRef(element, mount, onRef = assertDOMNode) {
const ref = React.createRef();
mount(<>{React.cloneElement(element, { innerRef: ref })}</>);
mount(<React.Fragment>{React.cloneElement(element, { innerRef: ref })}</React.Fragment>);
onRef(ref.current);
}
1 change: 0 additions & 1 deletion packages/material-ui/src/utils/deprecatedPropType.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
function deprecatedPropType(validator, reason) {
/* istanbul ignore if */
if (process.env.NODE_ENV === 'production') {
return () => null;
}
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/utils/requirePropFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
function requirePropFactory(componentNameInError) {
/* istanbul ignore if */
if (process.env.NODE_ENV === 'production') {
return () => null;
}
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui/src/utils/unsupportedProp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
function unsupportedProp(props, propName, componentName, location, propFullName) {
/* istanbul ignore if */
if (process.env.NODE_ENV === 'production') {
return null;
}
Expand Down