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

[core] Replace warning with manual console.error #17404

Merged
merged 19 commits into from
Sep 13, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Sep 12, 2019

Review on per-commit basis is advised.

Replace problematic warning(condition, message) with

// __DEV__ is a magic identifier that is replaced during transpilation with `process.env.NODE_ENV !== 'production'`
// It just tries to help readability/reduce amount to write
if (__DEV__) {
  if (!condition) {
    console.error(message)
  }
}
  1. double if helps dev readability (code folding, separate fail condition with environment invariant)
  2. warning tripped me personally all the time and I suspect others as well: does this warn if the condition is true or false?1
  3. Allows customizing when we want to console.warn or console.error
  4. micro-optimization for dev bundles by evaluating warning messages only if they'll actually surface.

Point 3 is the main selling point here. warning using console.error and starting the message with Warning was always confusing. We don't need to propagate this problematic choice by using even more fb code but rather use the platform specific methods (console.error and console.warn).

This PR is part of a larger effort to improve DX which is split up to allow review.

1
It automatically removes double negations if you had warning(!children, 'children should not be provided') which became if !!children then log a warning in your head.

Help with #15343

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 12, 2019

Details of bundle changes.

Comparing: 6a65aa7...ceafa54

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.03% -0.07% 331,787 331,688 90,602 90,543
@material-ui/core/Paper -0.25% -0.22% 68,813 68,644 20,494 20,449
@material-ui/core/Paper.esm -0.16% -0.16% 62,185 62,083 19,222 19,192
@material-ui/core/Popper -0.24% -0.18% 28,466 28,397 10,177 10,159
@material-ui/core/Textarea 0.00% -0.05% 5,082 5,082 2,133 2,132
@material-ui/core/TrapFocus -1.77% -1.30% 3,834 3,766 1,617 1,596
@material-ui/core/styles/createMuiTheme -0.47% -0.51% 16,410 16,333 5,838 5,808
@material-ui/core/useMediaQuery -2.74% -1.50% 2,558 2,488 1,066 1,050
@material-ui/lab -0.08% -0.07% 153,690 153,573 46,768 46,733
@material-ui/styles -0.17% -0.13% 51,508 51,420 15,308 15,288
@material-ui/system -0.56% -0.23% 15,668 15,581 4,361 4,351
Button -0.13% -0.13% 78,700 78,595 24,045 24,014
Modal -0.51% -0.37% 14,601 14,527 5,121 5,102
Portal 0.00% -0.30% 2,907 2,907 1,322 1,318
Rating -0.15% -0.17% 70,053 69,948 21,881 21,844
Slider -0.14% -0.14% 75,174 75,069 23,206 23,173
colorManipulator -1.77% -1.56% 3,904 3,835 1,543 1,519
docs.landing 0.00% 0.00% 52,232 52,232 13,768 13,768
docs.main -0.05% -0.07% 597,448 597,147 190,763 190,633
packages/material-ui/build/umd/material-ui.production.min.js +0.01% 🔺 0.00% 302,596 302,622 86,953 86,957

Generated by 🚫 dangerJS against ceafa54

@eps1lon eps1lon marked this pull request as ready for review September 12, 2019 14:44
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

  1. Sounds good
  2. You, me and many more from what I have heard so far :)
  3. Nice, should we do a follow-up effort to decide when using warning and error?
  4. I'm happy to see one less dependency in the package.json :)

I have mixed feelings about __DEV__, I like how it's shorter to write, how it matches what React does but I dislike the extra transformation step, making people wonder how it will be transpiled. Overall, it might have more pros than cons.

docs/src/modules/utils/helpers.js Outdated Show resolved Hide resolved
@@ -110,7 +109,8 @@ function attach({ state, theme, stylesOptions, stylesCreator, name }, props) {
...options,
});

warning(props, 'Material-UI: props missing.');
// never true
// warning(props, 'Material-UI: props missing.');
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we could easily fix it by removing the default prop value (in return (props = {}) => {).

Copy link
Member Author

Choose a reason for hiding this comment

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

useStyles() is fairly common looking at all the TS issues. Supporting this makes sense since most styles aren't dynamic. I would however question why we need the default value anyway. Rather we should let this flow through and either let it crash with the usual null-pointer exception and rely on a typed language or throw a custom TypeError.

packages/material-ui/src/Breadcrumbs/Breadcrumbs.js Outdated Show resolved Hide resolved
);
});

CardMedia.propTypes = {
/**
*
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a description? It always feels weird to me when I have to add one for this prop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sure.

Children should always be explicit. There are a lot of misconceptions about this prop which mostly stem from people assuming they "just work". This is bascially true for every component in this library but only because our components are very close to the DOM. The bigger the component the less obvious what kind of children are handled if any.

packages/material-ui/src/styles/createSpacing.js Outdated Show resolved Hide resolved
packages/material-ui-styles/src/withStyles/withStyles.js Outdated Show resolved Hide resolved
packages/material-ui-styles/src/withStyles/withStyles.js Outdated Show resolved Hide resolved
packages/material-ui-styles/src/withTheme/withTheme.js Outdated Show resolved Hide resolved
packages/material-ui/src/styles/createPalette.js Outdated Show resolved Hide resolved
packages/material-ui/src/utils/helpers.js Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Sep 12, 2019
@eps1lon
Copy link
Member Author

eps1lon commented Sep 13, 2019

I have mixed feelings about DEV

Me too, let's kill it. It would be simpler with rollup but since it's just a write-helper I'm not too attached to it.

@eps1lon eps1lon merged commit 2b1bad3 into mui:master Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants