[docs] Improve jsdom section

[oliviertassinari]

I noticed the casing issue while reading https://next.mui.com/material-ui/migration/upgrade-to-v9/#jsdom-support. So I went to fix the case.

While I read the sentence, I was confused about the copies, e.g., "for for", line break for each sentence, use of future tense, use of we, etc. So I fixed those.

As for the change itself, looking closer, I don't see how it's possible to only use NODE_ENV for tree shaking. It seems wrong, we need process.env.NODE_ENV to represent the environment, 'development', 'test', 'production', so I have softened the messaging.

Why:

1. Expired annual license after the grace period that runs in dev mode should throw (but not in prod or test env). https://github.com/mui/mui-x/blob/71ec303a389dacdf76c3738c808c31837d68c4d9/packages/x-license/src/verifyLicense/verifyLicense.ts#L274
2. Having to wait for the animation to complete in test mode feels like hell. The developer not having to wait sounds like a better default. How do we know that the environment is in test mode? process.env.NODE_ENV is that. For example:
   https://github.com/mui/mui-x/blob/7dc081233c1fb95afa9f8131d2c48a762fc6f716/packages/x-charts/src/internals/plugins/corePlugins/useChartAnimation/useChartAnimation.ts#L79

This policy seems compatible with https://nodejs.org/en/learn/getting-started/nodejs-the-difference-between-development-and-production. And mui/mui-x#1151 touches on the need to make the default test DX great.

Context:

• [code-infra] Detect browser envs that don't support layout #47813
• [code-infra] Remove usage of NODE_ENV=test #47692
• [code-infra] Remove NODE_ENV as a check for test environment mui-x#21121

Preview: https://deploy-preview-48098--material-ui.netlify.app/material-ui/migration/upgrade-to-v9/#jsdom-support

[mui-bot]

Netlify deploy preview

• docs/data/material/migration/upgrade-to-v9/upgrade-to-v9.md

Bundle size report

Bundle	Parsed size	Gzip size
@mui/material	0B(0.00%)	0B(0.00%)
@mui/lab	0B(0.00%)	0B(0.00%)
@mui/system	0B(0.00%)	0B(0.00%)
@mui/utils	0B(0.00%)	0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 008a4e9

[Janpot]

we need process.env.NODE_ENV to represent the environment, 'development', 'test', 'production',

The ecosystem standardized on production/development to disable debug logs, enable optimizations, disable caches,... These behaviors must be testable. therefore the test suite can't overload the environment selection with its own logic. If you want to branch on "test environment" in end-user code (avoid at all cost), you should use a different signal. i.e. There is no wide-spread consensus on NODE_ENV=test and it's just a bad idea in the first place.

2. Having to wait for the animation to complete in test mode feels like hell. The developer not having to wait sounds like a better default. How do we know that the environment is in test mode? process.env.NODE_ENV is that. For example:

One of:

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

This policy seems compatible with https://nodejs.org/en/learn/getting-started/nodejs-the-difference-between-development-and-production.

It says:

Therefore, setting NODE_ENV to anything but production is considered an antipattern.

This is close to what I'm proposing, we only compare NODE_ENV with 'production' and ignore any other possible values.

Expired annual license after the grace period that runs in dev mode should throw (but not in prod or test env).

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.

[oliviertassinari]

There is no wide-spread consensus on NODE_ENV=test and it's just a bad idea in the first place.

@Janpot It feels that it's different in practice. NODE_ENV=test is usually the default when running tests:

• Vitest. https://github.com/vitest-dev/vitest/blob/e92f2878e6a097cf5b68735dc005d90bee06c304/packages/vitest/src/node/cli/cli-api.ts#L162
• Jest. https://github.com/jestjs/jest/blob/399b831484e372c07e03a634a197f3b9f5a261f7/packages/jest-cli/bin/jest.js#L13
• bun test. https://github.com/oven-sh/bun/blob/2e610b1402f5cc17f11803b01b87cbc59f563fab/src/cli/test_command.zig#L1427
• node test. Doesn't set NODE_ENV
• Playwright. Doesn't run the code directly. Doesn't set NODE_ENV, only set https://github.com/microsoft/playwright/blob/ae521ddf1a8b3bb781f76f02ff87ac4462438c8a/packages/playwright-core/src/server/utils/env.ts#L57

On the opposite side of the coin, there is something I don't understand. What's the value of never testing NODE_ENV === 'test'.

To what I understand, we use this check to improve the default experience for people who test the wrapped version of our products. It avoid the need for people to configure what they would want 90% of the time. But we still make it possible to test a different behavior.

[Janpot]

NODE_ENV=test is usually the default when running tests:

The fact that 5 environments do 3 different things confirms to me that there is no consensus. To note that if we strictly rely on comparing only with 'production', the behavior will be identical across all 5 environments.

To what I understand, we use this check to improve the default experience for people who test the wrapped version of our products. It avoid the need for people to configure what they would want 90% of the time. But we still make it impossible to test a different behavior.

To me this is the problem: 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. We have been running the jsdom logic in our browser tests for years. Even though they support layout.

As the Node.js doc you mention points out, environment checks are just bad architecture. It assumes that everyone's testing environment and requirements are the same. Better to build individual switches if you need specific features turned off/on.

[oliviertassinari]

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?

[Janpot]

Expired annual license after the grace period that runs in dev mode should throw (but not in prod or test env).

Why not in test env? We should give the users the choice: You want to verify your license is still valid in your tests? Great, add a license to your test env. You don't care and don't want a renovate update be flagged when it breaks the license? Also great, suppress the warning in your test env.
Our libs shouldn't be suppressing warnings, that limits the choice of our users. Our user's test envs need to suppress warnings, if they don't support certain features, or if they don't want to test for that specific behavior. Not using NODE_ENV=test is giving more power to our users.

[oliviertassinari]

Why not in test env?

I have moved this thread to the initial change https://github.com/mui/mui-x/pull/21121/changes#r2991594432.

---

So overall, it looks like we should work toward:

The NODE_ENV variable will exclusively be used for for tree-shaking.

But we are not there yet, we might even need the test frameworks to change some of the way their code behaves.

[brijeshb42]

I see that we do vale validation through both Github actions and also run it in CircliCI. I know that running on GH provides nice annotations within the code review UI. Do we want to do this in both CIs or can we just keep one of these ?