Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

useReducedMotion: Guard process usage #16679

Closed
wants to merge 3 commits into from
+16 −1
Closed

useReducedMotion: Guard process usage #16679

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7380adb
Guard process.env.FORCE_REDUCED_MOTION usage
sirreal Jul 19, 2019
96bcae2
Use try/catch to allow for defining bundlers
sirreal Jul 19, 2019
b4671b9
Add comment about why `try/catch` was used
sirreal Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/compose/src/hooks/use-reduced-motion/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,28 @@ import useMediaQuery from '../use-media-query';
*/
const IS_IE = window.navigator.userAgent.indexOf( 'Trident' ) >= 0;

/**
* Force reduced motion behavior at build time
*
* @type {boolean}
*/
let FORCE_REDUCED_MOTION = false;

// Prefer the try/catch to allow bundlers to replace `process.env.FORCE_REDUCED_MOTION` completely.
// Checks like `typeof process === 'object' && …` will be safe, but may short-circuit if a bundler
// has replaced `process.env.FORCE_REDUCED_MOTION` with `true`, but `window.process`
// remains undefined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

try {
FORCE_REDUCED_MOTION = !! process.env.FORCE_REDUCED_MOTION;
} catch ( err ) {}

/**
* Hook returning whether the user has a preference for reduced motion.
*
* @return {boolean} Reduced motion preference value.
*/
const useReducedMotion =
process.env.FORCE_REDUCED_MOTION || IS_IE ?
FORCE_REDUCED_MOTION || IS_IE ?
() => true :
() => useMediaQuery( '(prefers-reduced-motion: reduce)' );

Expand Down