[docs] Improve jsdom section by oliviertassinari · Pull Request #48098 · mui/material-ui (original) (raw)

We used this check to tune the library for browsers that don't support layout. We naively used NODE_ENV=test as a signal for those browsers. Except, now comes karma or vitest browser mode along, which also sets NODE_ENV=test, but also does support layout since running in a real browser

@Janpot This aspect does seem like it's more correct code behavior, agree.
I believe the markdown update I did in this PR fully relies on this, it's better.

To note that if we strictly rely on comparing only with 'production', the behavior will be identical across all 5 environments.

But I'm still confused. When I try to understand why we would not have a single NODE_ENV=test branch in the code, the only logic I can find is: it's about making it easier for us, maintainers, we would have fewer combinations to think about. But this comes at the expense of making the default behavior worse for the developers because they would need to configure more things.

If this is desired behavior, then it needs to be testable. i.e. test suite needs to turn on/off dev mode and verify the difference in behavior.

In this case (license verificator), to test the behavior of the code that would happen in "prod", we need to change process.env.NODE_ENV to set it to "test".

You respect prefers-reduced-motion in your code and emulate in your test env […] You provide a global config to turn off animations

This feels like more complexe that it needs to be, if process.env.NODE_ENV === 'test', I would expect that most developers don't want to waste test time or have a bunch of await for the animations. But OK, I could also see that as being perceived as "magic", in practice, if tests are barely slower, and people need to write them async anyway (I don't believe this hypothesis is true, at all), it's the same value delivered, with the advantage of being more predictable for developers (less magic).

Overall, when we look at all the places where we have process.env.NODE_ENV === 'test' in MUI X (mui/mui-x#21121), we seem to have:

  1. license verificator: It's valid, covered above.
  2. getStringSize(): It seems invalid; we are missing a test teardown? [charts-pro] Update zoom using requestAnimationFrame mui-x#17137 (comment)
  3. animation: Not so clear, covered above.
  4. grid focus warning skipped: It seems invalid; there is a bug to fix. But if it can't be fixed, what's the alternative?
  5. scheduler scroll: It seems invalid, there is a bug to fix. But if it can't be fixed, what's the alternative?
  6. useResizeObserver: It seems invalid, there is a bug to fix. But if it can't be fixed, what's the alternative?
  7. x-telemetry: It seems valid, for the same reason as 1.

So overall, removing "The NODE_ENV variable will exclusively be used for for tree-shaking" from the docs seems correct, hence no blocker with this PR?