Guard access to browser globals #16227

blowery · 2019-06-19T21:18:42Z

Description

This allows the module to be used from environments other than the browser, like plain node, opening up server side rendering.

The problem occurs because we try to reference browser globals during module initialization. This PR defers usage to when the the references are actually needed, which at least lets the modules initialize. Many will still fail when run in a non-browser-like environment, but this may be good enough to allow some basic components to server-side render with React.

How has this been tested?

Just the existing unit tests.

Types of changes

Bug fix

Checklist:

My code is tested. (The original code has no tests)
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.

Fixes #9265

This allows the module to be used from environments other than the browser, like plain node, opening up server side rendering.

gziolo · 2019-06-20T09:58:22Z

I'm wondering why package-lock.json get included here as well. It seems like they shouldn't change at all.

…h window

blowery · 2019-06-20T13:26:45Z

Found a bunch more and updated the PR. I found instances by running node -r "@wordpress/components" and kept chasing until node would happily boot into the REPL.

blowery · 2019-06-20T13:27:31Z

@gziolo looks like updates because I ran npm i on a Mac. Reverted it back to what's on master.

blowery · 2019-06-20T13:29:48Z

packages/components/src/scroll-lock/index.js

@@ -15,7 +15,7 @@ import { Component } from '@wordpress/element';
 * @return {Component} The bound ScrollLock component.
 */
 export function createScrollLockComponent( {
-	htmlDocument = document,
+	htmlDocument = typeof document !== 'undefined' ? document : undefined,


This one is only required because we create a "default" scroll lock component at the end of the file. Perhaps we could create it on-demand instead or creating it during module init?

blowery · 2019-06-20T13:30:34Z

packages/components/src/keyboard-shortcuts/index.js

-import 'mousetrap/plugins/global-bind/mousetrap-global-bind';
+if ( typeof window !== 'undefined' ) {
+	// only initialize the global bind if we're in a browser environment
+	require( 'mousetrap/plugins/global-bind/mousetrap-global-bind' );


I hate this, but I'm unsure how else to handle it. Mousetrap returns early when window isn't present, but the plugin has no such protection.

Should probably be fixed upstream?

Should probably be fixed upstream?

If there was an issue, I would suggest so.

Looking at the code, it's not immediately obvious to me what the issue is.

There's an alternative that we copy and adapt the code from the plugin directly in this file.

I don't have a strong opinion on approach, even as implemented here. Would be good to have a better understanding of the problem this is addressing though.

Worth starting by opening an issue at upstream? https://github.com/ccampbell/mousetrap/issues

Should probably be fixed upstream?

If there was an issue, I would suggest so.

Looking at the code, it's not immediately obvious to me what the issue is.

There's an alternative that we copy and adapt the code from the plugin directly in this file.

I don't have a strong opinion on approach, even as implemented here. Would be good to have a better understanding of the problem this is addressing though.

My (admittedly blunt) take is that if it's available as an npm, it should at least not break things when used on the server side (read: node) but fall back to a no-op.

And as Ben points out, mousetrap itself takes precautions to prevent that from happening -- it's just the global-bind plugin that doesn't.

Precautions against what? I see no references to browser globals in the global-bind plugin code.

The plugin goes after the Mousetrap global and the Mousetrap.prototype, but mousetrap returns early when window is not present and never sets the Mousetrap global. So it's not a browser global that's missing, it's Mousetrap.

filed ccampbell/mousetrap#465 upstream

blowery · 2019-06-20T13:30:59Z

packages/components/src/focusable-iframe/index.js

@@ -35,7 +29,7 @@ class FocusableIframe extends Component {
 			return;
 		}

-		const focusEvent = new FocusEvent( 'focus', { bubbles: true } );
+		const focusEvent = new window.FocusEvent( 'focus', { bubbles: true } );


We don't technically need the window here, but it was failing lint without it.

blowery · 2019-06-20T13:31:43Z

packages/dom-ready/src/index.js

@@ -15,6 +15,9 @@
 * @return {void}
 */
 const domReady = function( callback ) {
+	if ( typeof document === 'undefined' ) {
+		return;


should we call the callback instead? warn? assume folks using this are using it in an environment that supports the dom?

should we call the callback instead? warn? assume folks using this are using it in an environment that supports the dom?

Hm. I think the callback should be called, yes. I don't feel strongly about it, but I think a warning message would be reasonable as well to inform the developer that a document was not found.

Hm. I think the callback should be called, yes. I don't feel strongly about it, but I think a warning message would be reasonable as well to inform the developer that a document was not found.

We should implement this.

blowery · 2019-06-20T13:34:31Z

packages/dom/src/dom.js

@@ -40,11 +28,11 @@ function isSelectionForward( selection ) {
 	/* eslint-disable no-bitwise */
 	// Compare whether anchor node precedes focus node. If focus node (where
 	// end of selection occurs) is after the anchor node, it is forward.
-	if ( position & DOCUMENT_POSITION_PRECEDING ) {
+	if ( position & anchorNode.DOCUMENT_POSITION_PRECEDING ) {


you'll see this pattern a lot through this PR. Instead of using the constant hanging off window.Node, use the same constant which is present on anything that implements Node.

Nice! There are many more instances of this across the codebase. Could they all be replaced in this PR?

I wanted to make sure this was an acceptable approach before pushing it further. The alternative would be to use Node.DOCUMENT_POSITION_PRECEDING, just not as a module level variable.

This is a great approach!

blowery · 2019-06-20T13:35:53Z

packages/priority-queue/src/index.js

+	if ( typeof window !== 'undefined' ) {
+		return window.requestIdleCallback ? window.requestIdleCallback : window.requestAnimationFrame;
+	}
+	return setTimeout;


I'm just assuming falling back to setTimeout is ok here. The API to setTimeout includes an optional second parameter, the timeout, which we are not specifying at the call site, but which defaults to 0.

I'm just assuming falling back to setTimeout is ok here. The API to setTimeout includes an optional second parameter, the timeout, which we are not specifying at the call site, but which defaults to 0.

I think it should be fine. Not super problematic since it's internal-only, but we could wrap setTimeout if we wanted to avoid issues of whether the interface is capable of handling a second argument.

Suggested change

return setTimeout;

return ( callback ) => setTimeout( callback, 0 );

we could wrap setTimeout if we wanted to avoid issues of whether the interface is capable of handling a second argument.

I would suggest this be implemented.

aduth · 2019-06-24T20:54:32Z

@gziolo looks like updates because I ran npm i on a Mac. Reverted it back to what's on master.

Based on the specific changes, I suspect more likely because you're not running the latest version of NPM.

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/getting-started.md

aduth

For some of this, it seems to give us safety only on the relatively fragile basis that the code is less likely but to be run as in the case where it occurs at top-level scope. I suppose it's an improvement, but it's not a guarantee, and it could be argued we sacrifice some readability in the process.

Granted, I'm not sure what the alternative is, other than not referencing these properties, or some pattern of shimming values to guarantee they're at least defined (jsdom-like, or as we had before with modules importing element-closest if they used Element#closest).

aduth · 2019-06-24T20:55:59Z

packages/components/src/scroll-lock/index.js

@@ -15,7 +15,7 @@ import { Component } from '@wordpress/element';
 * @return {Component} The bound ScrollLock component.
 */
 export function createScrollLockComponent( {
-	htmlDocument = document,
+	htmlDocument = typeof document !== 'undefined' ? document : undefined,


An issue I see here is that it's prone to thrown errors, since we don't guard property reference in its use later in this function (and, as documented by the JSDoc, it's not nullable/"undefined"-able).

should we throw instead?

ah, right, I remember why I didn't add a throw: We create a default scroll lock component at the bottom of this file, so it's guaranteed to throw. Maybe we don't create that default component when window is missing? I'm not sure.

Maybe we don't create that default component when window is missing? I'm not sure.

Yeah, in a non-DOM context, I think changing the behavior here to be basically "return a noop function" would be least problematic.

Something like:

if ( typeof document === 'undefined' ) { return function ScrollLock() { return null; }; }

Tug · 2019-06-25T15:04:48Z

gziolo requested a review from hypest yesterday

Replying here for @hypest since he asked me to have a look

Changes look good and will not cause any problem to mobile native 👍

blowery · 2019-06-25T15:30:21Z

For some of this, it seems to give us safety only on the relatively fragile basis that the code is less likely but to be run as in the case where it occurs at top-level scope. I suppose it's an improvement, but it's not a guarantee, and it could be argued we sacrifice some readability in the process

This was just enough to get the code to load without throwing in a non-DOM environment. If we want real safety, that's a much larger PR and probably a change to how Gutenberg is approaching development.

ockham · 2019-07-17T09:48:08Z

This is blocking work in Calypso that would allow us to break out more components into isolated packages (Automattic/wp-calypso#34277), so I'd be highly interested in getting this merged 😬

@aduth Since Ben is out for the near future -- anything I can do to get this landed? Seems like a few of these changes should be fairly uncontroversial; for the rest, it'd probably be good to consider the circumstances under which the affected code is called (or isn't). Specifically -- is this good enough as a first iteration, or is there anything you'd like to see changed specifically?

aduth · 2019-07-24T17:45:34Z

@ockham I added a few comments for things which should be implemented here, if you can help take them on. I think it might also be worth doing a rebase to see if there are more recent additions of document references that we need to handle.

hypest · 2020-02-21T10:57:35Z

This PR seems to have become stale. Can it be closed @blowery?

blowery · 2020-03-02T15:29:21Z

Shutting this one down. It's quite stale.

aduth · 2020-03-02T15:36:05Z

Would I be correct in assuming that the goal of this pull request is still desirable (i.e. allowing code to at least be imported without errors in Node)?

blowery · 2020-03-02T17:07:05Z

@aduth yes, I think that's still a worthwhile goal

Guard access to window global in priority-queue

dd6c00a

This allows the module to be used from environments other than the browser, like plain node, opening up server side rendering.

blowery requested review from aduth, gziolo and youknowriad as code owners June 19, 2019 21:18

gziolo added [Type] Bug An existing feature does not function as intended [Package] Priority Queue /packages/priority-queue labels Jun 20, 2019

Use constants defined on the node prototype instead of running throug…

b16caa9

…h window

blowery requested review from daniloercoli, ellatrix, jorgefilipecosta and SergioEstevao as code owners June 20, 2019 11:33

Remove module level usage of browser globals

67e5d1b

blowery requested review from ajitbohra, chrisvanpatten, oandregal and talldan as code owners June 20, 2019 13:24

revert package-lock change

da902e7

blowery commented Jun 20, 2019

View reviewed changes

blowery changed the title ~~Guard access to window global in priority-queue~~ Guard access to browser globals Jun 20, 2019

gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Type] Build Tooling Issues or PRs related to build tooling and removed [Package] Priority Queue /packages/priority-queue labels Jun 24, 2019

gziolo requested a review from hypest June 24, 2019 11:43

aduth reviewed Jun 24, 2019

View reviewed changes

This was referenced Jun 26, 2019

Add Muriel TextControl to calypso-ui Automattic/wp-calypso#34277

Closed

Add additional WooCommerce onboarding styling to connection flow Automattic/wp-calypso#34380

Merged

ockham mentioned this pull request Jul 17, 2019

Build: Server-side Render UpgradeNudge for use in PHP Automattic/jetpack#13070

Merged

3 tasks

sirreal mentioned this pull request Jul 30, 2019

useReducedMotion: Guard process usage #16679

Closed

5 tasks

sirreal mentioned this pull request Aug 26, 2019

useReducedMotion hook: guard the reference to window #17165

Merged

blowery mentioned this pull request Nov 20, 2019

Move composite-checkout-wpcom into client/lib Automattic/wp-calypso#37729

Closed

This was referenced Jan 22, 2020

fix(deps): update wordpress monorepo Automattic/wp-calypso#38880

Merged

Make global-bind plugin safe in node envs ccampbell/mousetrap#465

Merged

aduth mentioned this pull request Feb 25, 2020

Block Editor: Simulated Media Query: Skip CSS rule if not style rule #20226

Merged

aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 25, 2020

blowery closed this Mar 2, 2020

blowery deleted the fix/unguarded-window-access branch March 2, 2020 15:29

jeherve mentioned this pull request May 22, 2020

Mousetrap: update to latest stable version #22548

Merged

ockham mentioned this pull request Aug 21, 2020

Site Editor on WP.com: Initial explorations, take two Automattic/wp-calypso#44928

Draft

1 task

	return setTimeout;
	return ( callback ) => setTimeout( callback, 0 );

Guard access to browser globals #16227

Guard access to browser globals #16227

Conversation

blowery commented Jun 19, 2019 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

gziolo commented Jun 20, 2019

blowery commented Jun 20, 2019

blowery commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham Jul 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jun 24, 2019

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tug commented Jun 25, 2019 • edited Loading

blowery commented Jun 25, 2019

ockham commented Jul 17, 2019 • edited Loading

aduth commented Jul 24, 2019

hypest commented Feb 21, 2020

blowery commented Mar 2, 2020

aduth commented Mar 2, 2020

blowery commented Mar 2, 2020

blowery commented Jun 19, 2019 •

edited

Loading

ockham Jul 17, 2019 •

edited

Loading

Tug commented Jun 25, 2019 •

edited

Loading

ockham commented Jul 17, 2019 •

edited

Loading