useReducedMotion: Guard process usage

[sirreal]

Description

The useReducedMotion hook provided by @wordpress/compose has unguarded usage of the process global:

gutenberg/packages/compose/src/hooks/use-reduced-motion/index.js

Line 19 in 6ef0222

process.env.FORCE_REDUCED_MOTION || IS_IE ?

process is not a global variable in the browser. Depending on the way this code is bundled, it may work well at runtime, or it may error when the useReducedMotion module is evaluated.

In webpack, this is is controlled by the node configuration option. If you attempt to bundle a project like the following:

// webpack.config.js
module.exports = { node: false };

// src/index.js (entrypoint)
import { useReducedMotion } from '@wordpress/compose';

An error will be thrown at runtime when the script is evaluated:

Uncaught ReferenceError: process is not defined
    at Module.<anonymous> (main.js:38)
    at n (main.js:1)
    at main.js:1
    at main.js:1

How has this been tested?

Local testing by manipulating the installed module in a minimal test repo.

Types of changes

Bug fix - Unprotected process global usage could throw runtime errors when.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[youknowriad]

That makes sense, but it also probably goes against the fact that bundlers try to also tree-shake these. For instance here, the try/catch might be kept (needs testing) while otherwise, the check is completely removed. Thoughts?

[sirreal]

A better approach may be to strip this entirely from the published package as I mentioned below, but I'm not sure.

Specifically on this point, here are my thoughts:

• Requiring package consumers to have a special build setup seems unreasonable, so unguarded process.env.FORCE_REDUCED_MOTION seems unreasonable.
• Ideally, this would be determined at build time, replaced by a constant, and dead code elimination could remove parts of the code. That would require a construct like if ( process.env.FORCE_REDUCED_MOTION ) { /* block could be dead-code eliminated */ }. That doesn't seem possible given the previous point.
• In order to provide safety for package consumers (guard the process usage) without requiring a special build setup, that would require something like:

  if (
    typeof process === 'object' &&    // Check will remain at runtime
    process.env &&                    // Check will remain at runtime
    process.env.FORCE_REDUCED_MOTION  // Candidate for build-time replacement, may just be `true` or `false`
  ) {
    // Dead code elimination is very unlikely to occur here.
  }

  This seems to indicate that we can't have both safety and replacement with dead code elimination. Additionally, depending on the build setup, it's entirely possible to have process be undefined, but process.env.FORCE_REDUCED_MOTION replaced with true, which would make this snippet completely broken.
• If we accept the above points, that seems to leave us with just the try/catch version. A consumer could opt-in to the FORCE_REDUCED_MOTION, consumers don't need any specific build/bundle setup, and there's only a minimal bundle/runtime penalty of a few bytes of JavaScript and a try/catch check on module evaluation. This seems to be the only reasonable approach to me, unless we can completely remove this from the published package.

[youknowriad]

Thanks for the detailed explanation. That makes sense. The reason we have this env variable is that we want to disable animations in e2e tests. The ideal scenario would be to allow disabling animations at runtime. I didn't find a way to do it at that time.

[aduth]

process is not a global variable in the browser. Depending on the way this code is bundled, it may work well at runtime, or it may error when the useReducedMotion module is evaluated.

Can you elaborate on this? We have full control over how it is bundled. In what real-world scenarios are you considering it otherwise be bundled?

[sirreal]

process is not a global variable in the browser. Depending on the way this code is bundled, it may work well at runtime, or it may error when the useReducedMotion module is evaluated.

Can you elaborate on this? In what real-world scenarios are you considering it otherwise be bundled?

I ran into this in a real world scenario:

Automattic/wp-calypso#34217 (comment)

A package upgrade resulted in a broken build artifact with no build-time errors. The problem was fortuitously caught by e2e testing.

The inclusion of unguarded process.env[ ANYTHING ] requires consumers to either polyfill process at runtime, or replace all of the properties at build time with something like webpack's DefinePlugin.

React and others do rely on process.env, most notably process.env.NODE_ENV. This is widespread enough that webpack handles it for you:

Many libraries will key off the process.env.NODE_ENV variable to determine what should be included in the library. […] Since webpack v4, specifying mode automatically configures DefinePlugin for you

While process.env.NODE_ENV is a pseudo-standard, I don't believe we should extensively add to process.env because it requires package consumers to use specific build setups. At the very least, it would be good to provide clear information on the constraints and instructions on how to use this package with the most common bundlers.

---

We have full control over how it is bundled.

I don't understand what you mean by this. Anyone consuming the @wordpress/compose package will be in control of their own build system or bundler. The fact that process.env.FORCE_REDUCED_MOTION is used unguarded puts the onus onto package consumers to polyfill process or replace process.env.FORCE_REDUCED_MOTION as part of their build. If they do not, runtime errors may occur. This requirement appears to be a breaking change from @wordpress/[email protected] when hooks were added.

---

I'm not sure what the original intentions of using process.env.FORCE_REDUCED_MOTION to override the media query were, perhaps it was only meant for easy testing within Gutenberg and not for consumers. In that case, substituting false for process.env.FORCE_REDUCED_MOTION in the published package may be a good solution in that case. @youknowriad can you elaborate?

[aduth]

We have full control over how it is bundled.

I don't understand what you mean by this.

To me, it's a point of setting expectations for the environments we're targeting with our published build artifacts. If our guarantee is that the main of our packages are Node scripts, and if process.env is a standard global property in a Node environment, then we've no commitment to guard against it being undefined when used in other environments. If and how various bundlers interoperate with Node scripts is their concern.

To be clear, I'm not arguing against making a change for these packages to be easier to use, but I do think we should have a clear understanding about expectations of our target environments. Should we guard against require? console? window? Why or why not?

[sirreal]

If our guarantee is that the main of our packages are Node scripts […]

My assumption has been that some packages are intended primarily for use in the browser and some are intended to be used in Node. #16227 highlights how many browser globals are used which are problematic in the Node context, such as for server side rendering.

I do think we should have a clear understanding about expectations of our target environments.

Some packages are build/development tooling like @wordpress/scripts and target Node. Other packages like @wordpress/components primarily target the browser.

Should we guard against require? console? window? Why or why not?

• console is available in all contexts I'm aware of. Its usage seems fine. Similar to setTimeout and other globals that are common to the browser and Node.
• window is only available in the browser. For browser-oriented packages it has been allowed, but it becomes problematic for testing and SSR which happen in Node (Guard access to browser globals #16227). Ideally its usage is guarded.
• process is only available in Node. process.env.NODE_ENV usage is inescapable in browser-oriented packages because React uses it and we rely on React. This should be documented if it is not already. Apart from this one exception, I'd discourage addition of Node globals to any packages that may be used in the browser as . Node globals can be used in @wordpress/scripts, but should be avoided in @wordpress/compose, @wordpress/element, @wordpress/components, etc. I'd propose that the only Node-specific global allowed in browser-oriented packages is exactly process.env.NODE_ENV.

require, import, import(), and export are all a bit special and I believe they're outside the scope of this discussion.

Portability and ease of use should drive the "why or why not" for all of these questions. If published packages use a subset of the language that is common to Node and the browser, it makes it easier to consume these packages. Globals that are not common to both environments should either be guarded or well documented and added only after careful consideration as these types of changes can cause hard to detect breakage because they depend on bundler configuration.

[sirreal]

Interesting read on how and why React uses process.env.NODE_ENV which seems relevant for this PR: How Does the Development Mode Work? — Overreacted.

[aduth]

Noting the pull request at #18453 would have removed this environment variable. As I understand, the work there is still desired but not achievable at the time.

[ZuBB]

2all who faced this issue: suggested workaround is here -- https://github.com/Automattic/wp-calypso/pull/34217/files#r310732226

2wp team: guys, please try to find some solution for this. 1+ year passed since PR was created

[Mamaduka]

The code in question no longer exists. See #34132.

I'm going to close the PR.