[core] Fix ReferenceError in browser builds without process polyfill #13164
Conversation
eps1lon
force-pushed
the
fix/core/browser-build-process
branch
from
8f2f579 to
8cf7abb
Compare
|
Maybe we shouldn't have |
|
@oliviertassinari I didn't know any other solution for our unit tests that didn't involve rewriting all of them. |
eps1lon
force-pushed
the
fix/core/browser-build-process
branch
from
8cf7abb to
0eae2c7
Compare
eps1lon
force-pushed
the
fix/core/browser-build-process
branch
from
0eae2c7 to
7c1ee6f
Compare
@eps1lon The current solution is hiding valuable warnings. For instance, yesterday, I have discovered an issue with the legacy button variants: https://github.com/mui-org/material-ui/pull/13158/files#diff-4175d84e63a1a5a340986f250b17c796R108. |
| @@ -227,13 +227,13 @@ function Button(props) { | |||
| } = props; | |||
|
|
|||
| warning( | |||
| process.env.MUI_SUPPRESS_DEPRECATION_WARNINGS || variant !== 'flat', | |||
| getEnv('MUI_SUPPRESS_DEPRECATION_WARNINGS') || variant !== 'flat', | |||
There was a problem hiding this comment.
I think that we can safely remove MUI_SUPPRESS_DEPRECATION_WARNINGS here. I have already updated the tests to handle it.
| @@ -27,7 +28,7 @@ export default function createTypography(palette, typography) { | |||
| // Tell Material-UI what's the font-size on the html element. | |||
| // 16px is the default font-size used by browsers. | |||
| htmlFontSize = 16, | |||
| suppressDeprecationWarnings = process.env.MUI_SUPPRESS_DEPRECATION_WARNINGS, | |||
| suppressDeprecationWarnings = getEnv('MUI_SUPPRESS_DEPRECATION_WARNINGS'), | |||
There was a problem hiding this comment.
suppressDeprecationWarnings is only used in the Typography component. moving MUI_SUPPRESS_DEPRECATION_WARNINGS there helps with code splitting.
|
@oliviertassinari Deprecation warnings are not something that needs fixing in our codebase at this moment. They are only for the user that want to prepare for the next major. If they don't want to migrate then they can simply suppress deprecation warnings and pin to MAJOR. |
@eps1lon We have left variants behind when introducing the warnings. I looking for a solution to limit this possibility as much as possible. Maybe we should collect the warnings in the visual regression tests and throw if we see one.
Have you ever seen React providing a way (in the public API) to suppress deprecation warnings? Not I'm aware of. It's done on purpose. If I had to guess it's because it's an incentive for people to upgrade. |
The button variant deprecation did not need the env variable, correct. For symmetry I wanted users to have the opportunity for every deprecation. I should've checked for remnants but since they were already documented as deprecated I assumed this was already done.
Then we don't do it the next time. There were no objections during review of the PR that introduced them. |
I'm only using this argument because we can leverage it to simplify the situation :) |
Closes #13163