Replace warning with tiny-warning

[TrySound]

• esm support
• not stuck with mirroring facebook internals once in a year
• jss v10 already migrated to it so size improvement will come in mui v4
• tiny-warning is really console.warn; red wall in console is scary

• I have followed (at least) the PR section of the contributing guide.

[oliviertassinari]

@TrySound From my end, the only argument I will consider is the bundle size, larger we close, smaller we merge. I don't think that any of the other dimension matters. Unless I'm missing one? It's a dev dependency designed to have a minimal overhead in production.

[TrySound]

@oliviertassinari I'm thinking about cleaning jss v9 from my bundle (via aliasing) and installing @material-ui/styles. Is it doable?

[oliviertassinari]

@TrySound Using @material-ui/styles means bundling the styles modules and JSS twice. Yes, you can force the yarn resolution to minimize the impact.

[oliviertassinari]

The size-limit stats using webpack (and not rollup). I believe it's using the esm version when available:

• before: Package size: 838 B
• after: Package size: 903 B

@TrySound What's the other advantage I miss? How does esm support matter? How is a package that hasn't changed for ten years that works an issue? React use console.error right? JSS can always go back to warning if it's better cc @kof.

[TrySound]

@oliviertassinari The size is changed because of jss miss. Some warnings are not wrapped with NODE_ENV condition. Ideally this will not change the size because it's always stripped from production builds.

An issue is commonjs which produces lots of bugs for us (I saw a lot of great projects with just overengineered exports). ESM is a good replacement. But we should replace cjs with it without waiting for miracle.

And as I said warning maintainers are stuck on facebook internals. They are mirroring facebook. They said so.

Are they the same? Nope. They just don't care much about it. Think it's dead.

https://github.com/BerkeleyTrue/warning/blob/master/warning.js
https://github.com/facebook/fbjs/blob/master/packages/fbjs/src/__forks__/warning.js

react-router and formik migrated too. Nobody regret about legacy.

[TrySound]

I see by the way warning package from react-event-listener appears in production bundle with interop. The size will be smaller in the end.

[oliviertassinari]

I see by the way warning package from react-event-listener appears in production bundle with interop. The size will be smaller in the end.

These stats are specifically

for this module:
https://github.com/mui-org/material-ui/blob/869d2699f42be84a3528f73080434a5b409b706c/packages/material-ui/src/styles/colorManipulator.js#L1-L251
We can ignore react-event-listener no?

[oliviertassinari]

I would vote for rejecting this change because:

• It's making having to handle console.warn and console.error. It's not consistent and introduces a testing overhead. React is using console.error
• It's increasing the bundle size.

[TrySound]

The funny thing uglify was able to inline constant flags but seems like that shit is broken again. Rollup is the best here. And it consumes only ESM.
https://github.com/alexreardon/tiny-warning/blob/master/src/index.js

React uses error for historical reasons. So it's a legacy we should not follow.

[TrySound]

console.warn didn't show stack trace long time ago. Times are changed. Let's not stuck with legacy and use semantical method. I would be really happy to distinct react/prop-types wall from any other package.

[mbrookes]

Personally, I think these PRs with negligibly smaller or negligibly more performant alternatives are a distraction we could do without. @TrySound please consider putting your time to something more productive.

[TrySound]

Fuck the ecosystem, somebody else will fix it, right? I'll see.

[kof]

I think the biggest point here is really to migrate to ESM, a few bytes doesn't matter here. I prefer in general to depend on better maintained and more modern packages. Also I preferred to not reject a PR because even if it doesn't matter that much, I always support people who care and if a person invests time to improve ecosystem, its worth supporting, even if this particular change isn't that important.

[mbrookes]

"If it ain't broke, don't fix it."

[TrySound]

[kof]

Its not broken enough to call it broken. Its really about ecosystem and people.

[mbrookes]

Since @TrySound can't engage in adult conversation and explain the problem this is trying to solve, and feels abusive language is the way to get this PR merged, that ends the discussion as far as I'm concerned.

[TrySound]

Sorry. I'm tired from these conversations. It feels like nobody listen to me. So I became angry.

My point as @kof said is ecosystem. Not just bytes. I want to move the ecosystem to a single and powerful module system which can be always statically analyzed and optimised.

So I'm tried to cover as much as possible packages in my node_modules with esm. Material-ui will end up with the same. This is the idea I promote for such packages #13391

If we stay on our chairs then programming doesn't make sense. We should move ecosystem forward to get rid from all complexities one day and start to solve new problems.

I like to solve technical debt. And I'm trying to solve it here. Will you let me do this?

[eps1lon]

Sorry. I'm tired from these conversations. It feels like nobody listen to me. So I became angry.

I can totally understand you because I have the same issue in #14152 with you. You keep saying that it matters if a single function module is in esm or cjs and I keep posting examples where this is not the case. If you want to move the ecosystem forward why not start with the people and explain how a single function module in esm contributes to the treeshakability of the hole bundle? If we could understand this then we can work on other modules where we are involved instead of only you going around and simply saying "do it this way. it's right.". I get your frustration but I'm also frustrated if I'm supposed to accept changes to problems that I can't reproduce.

I like to solve technical debt. And I'm trying to solve it here. Will you let me do this?

I'd like to move together. We don't have to all be able to implement the solution but we should know what the problem is and how you solve it.

Overall I do agree with moving some warnings to console.warn instead of console.error. react did the same and I would follow their lead that we should choose on a case by case basis if something should be logged as an error or warning.

The issue is that we can't really apply this to deprecation warnings in props here. propTypes are always logged with console.error. Manually using console.warn would not include the component stack which I value very high.

Some of the warnings currently would be better place in propTypes e.g. checking if children are fragments is IMO obviously props validation and should be done with propTypes.

[TrySound]

Alright, this is mostly political change.

ESM was suggested to classNames but still not accepted because of adoption fear. And this is a problem, without adoption there won't be adoption. Users won't will think it's okay to consume two different module systems which often cannot communicate well with each other. ESM was suggested to warning package but rejected because of absolutely no reason (BerkeleyTrue/warning#24 (comment)). I see it like "we will do what facebook does" -> "we are not interested in improving our facebook lib, look we removed it from react :)". Warning package has bugs but nobody interested in fixing it I see (BerkeleyTrue/warning#32). So I consider both projects dead.

ESM is important step for javascript. It opened the door for improvements which was not considered before or at least didn't have so much success like scope hoisting and treeshaking. It has important constraints like only top-level imports which forces us to consider different ways of dynamic imports like package.json fields and import() syntax. This prevents us from error prune runtime analyzing.

My app previously used webpack for bundling. Everything went good enough. Our bundle was 6mb (splitted with chunks) and it's ok. Then rollup introduced code splitting and I tried it. As a result I dropped 300kB from all chunks. But there was a problem with many packages. They use commonjs. And not in the best way.

For example react-map-gl had require calls in the middle of their esm. It was used to conditionally include module in browser or node. Here's the conditional function. It's full of runtime and I didn't trust it. It's not mirrored in a spec, it's not widely used convention. Just a hack. So getting rid from commonjs fixed the problem.
https://github.com/uber/react-map-gl/pull/599/files#diff-6fdd1b4a36be70b11237cd22a80289c3L6

Another example. CJS require from ESM interop does not always work. Because of this react-event-listener is still cjs.
webpack/webpack#6584

Why migrate to ESM? Well, at least to not have this in your bundle.
https://unpkg.com/@material-ui/core@3.8.3/colors/index.js
Also to get rid from all not controllable dynamism from our code.

Why migrate small one-function modules? This is political. Such big project like material-ui will force users to consume the same dependencies. Will force to consume more ESM and less CJS. This will move the whole community forward. I think after three years of spec it's reasonable goal.

ESM can be run out of the box in browser. Tools like unpkg resolves dependencies.
https://unpkg.com/jss@10.0.0-alpha.7/dist/jss.esm.js?module
This will not work for material-ui yet but I suggest you one step forward.

[oliviertassinari]

@TrySound Thank you for this summary. I'm closing for consistency with React usage of console.error.

• prop-types: console.error https://github.com/facebook/prop-types/blob/master/checkPropTypes.js#L19
• warningWithoutStack: console.error https://github.com/facebook/react/blob/b87aabdfe1b7461e7331abb3601d9e6bb27544bc/packages/shared/warningWithoutStack.js#L30

Overall I do agree with moving some warnings to console.warn instead of console.error. react did the same

It's partially true, most of their warnings are using console.error: warningWithoutStack(). However, you are right, they have a lowPriorityWarning() module that uses console.warn, only for non important warnings. If you grep the two methods in React's codebase, you will find warningWithoutStack x 59 and lowPriorityWarning x 12.

@TrySound Maybe tiny-warning could add an option to chose between warning and error?

[oliviertassinari]

Regarding the ESM vs CJS. I'm glad there are people like you @TrySound to promote its usage in the community. Thank you for that! It's going in the right direction. I wish we could just kill the CJS version, encourage people to use https://github.com/standard-things/esm on Node and move forward. But I doubt we can do that, yet.

Now, as a maintainer of Material-UI, I have limited bandwidth. I focus my energy on the problems that have solutions our users can benefit from the most. I believe that migrating warning to ESM is a nonproblem for our users. The project code hasn't changed for 10 years, it's consider a dead project, how cares? It works well, it's what is important. Please stop working on small packages that are easy to migrate that doesn't make a difference, it's a distraction keeping us busy. Let's work on the hard problems like #13391. This will make a difference :).

[oliviertassinari]

For more context on the long term vision for the warning package. I wish that https://github.com/oliviertassinari/babel-plugin-transform-dev-warning was able to remove the warning imports in production. I'm not sure if it's possible with ESM. Or https://github.com/4Catalyzer/babel-plugin-dev-expression, I can't remember why I have forked it.