Add keycodes file

[ellatrix]

Proposal to add key codes file

[youknowriad]

Do we really want a separate module for keycodes? This raises the question of whether we'd need a generic module: "utils", "common" or something like that.

[nylen]

General +1, but I don't think this should be its own Webpack entry point. Maybe ok for components, or maybe we need a utils build instead? (Note there is already wp.utils.WordCounter which we would have to be careful not to overwrite if it's loaded.)

See also #929 (introduced conflicts here, but should simplify the needed webpack.config.js changes - I'll rebase this PR shortly) and #941 for further discussion around imports.

[ellatrix]

Sorry, I don't know how web pack is configured etc. I just added it everywhere so that it works somehow. :) The point is that the codes are in one file. Where should the file be placed? cc @aduth

[ellatrix]

This branch will now fail. @nylen I don't know how to do add a new nodule now.

[ellatrix]

If not wp.utils what should we call it then? Is there any way we can extend with Webpack?

[ellatrix]

There is a way :) https://github.com/webpack-contrib/expose-loader/blob/master/index.js#L16
We'll need to export all the sub modules separately.

[nylen]

Extending wp.utils actually works fine as of the current state of this PR. word-count.js doesn't clobber window.wp.utils, and neither does the Webpack build edit: that second part was wrong:

this["wp"] = this["wp"] || {}; this["wp"]["utils"] =
/******/ (function(modules) { // webpackBootstrap
// ...

To demonstrate that this works well, load Gutenberg in the browser and inspect wp.utils before and after the following patch:

diff --git a/index.php b/index.php
index fcca4ad..c5bfa68 100644
--- a/index.php
+++ b/index.php
@@ -491,7 +491,7 @@ function gutenberg_scripts_and_styles( $hook ) {
 	wp_enqueue_script(
 		'wp-editor',
 		plugins_url( 'editor/build/index.js', __FILE__ ),
-		array( 'wp-api', 'wp-date', 'wp-i18n', 'wp-blocks', 'wp-element', 'wp-components', 'wp-utils' ),
+		array( 'wp-api', 'wp-date', 'wp-i18n', 'wp-blocks', 'wp-element', 'wp-components', 'wp-utils', 'word-count' ),
 		filemtime( plugin_dir_path( __FILE__ ) . 'editor/build/index.js' ),
 		true // enqueue in the footer.
 	);

[nylen]

The one thing I still don't like about how this PR is built is that importing from 'utils/keycodes' duplicates the keycodes code in the blocks and editor builds. My current thinking on how to remove this duplication would be something like this, which I suppose isn't awful, but I don't really want to do it here:

diff --git a/blocks/editable/index.js b/blocks/editable/index.js
index 6d2a16a..7d91c2c 100644
--- a/blocks/editable/index.js
+++ b/blocks/editable/index.js
@@ -11,7 +11,7 @@ import 'element-closest';
  * WordPress dependencies
  */
 import { Toolbar } from 'components';
-import { BACKSPACE, DELETE } from 'utils/keycodes';
+import { keycodes } from 'utils';
 
 /**
  * Internal dependencies
@@ -219,11 +219,11 @@ export default class Editable extends wp.element.Component {
 	onKeyDown( event ) {
 		if (
 			this.props.onMerge && (
-				( event.keyCode === BACKSPACE && this.isStartOfEditor() ) ||
-				( event.keyCode === DELETE && this.isEndOfEditor() )
+				( event.keyCode === keycodes.BACKSPACE && this.isStartOfEditor() ) ||
+				( event.keyCode === keycodes.DELETE && this.isEndOfEditor() )
 			)
 		) {
-			const forward = event.keyCode === DELETE;
+			const forward = event.keyCode === keycodes.DELETE;
 			this.onChange();
 			this.props.onMerge( forward );
 			event.preventDefault();
@@ -232,7 +232,7 @@ export default class Editable extends wp.element.Component {
 	}
 
 	onKeyUp( { keyCode } ) {
-		if ( keyCode === BACKSPACE ) {
+		if ( keyCode === keycodes.BACKSPACE ) {
 			this.onSelectionChange();
 		}
 	}

I hope we can find a way to preserve the 'utils/keycodes' import as I think it is much cleaner. Since there isn't much duplication in the build here, I'd propose that we discuss the import structure further on #941 and merge this as-is.

[ellatrix]

I don't really mind any way we can avoid it. If it has to be from utils first, maybe:

import { keycodes } from 'utils';

/* ... */

const { BACKSPACE, DELETE } = keycodes;

[ellatrix]

But yeah, I think it would be good to find a way to keep importing form utils/keycodes.

[nylen]

Or simply:

/**
 * WordPress dependencies
 */
const { BACKSPACE, DELETE } = wp.utils.keycodes;

... and wait for a future where WP has less global variables. 😭

[ellatrix]

@nylen Just noticed: it doesn't do window.wp.utils = window.wp.utils || {}; right? So the other way around won't work.

[nylen]

Ah right, good catch! So this means that the webpack version needs to be loaded first. We can accomplish this by adding wp-utils as a dependency of word-count.

This will mean that we'll be pulling in the word count code as well, which I think is ok, because we'll probably need it pretty soon.

Edit: or maybe this is reversed, I am getting confused again :(

In any case this is an example of something that needs a simple integration test to make sure it works both ways. See #939.

[ellatrix]

Yeah, either that, or exporting each thing we add to utils to window.wp.utils separately with Webpack.

[ellatrix]

... and wait for a future where WP has less global variables.

@nylen I was thinking about this more last night, and I'm not sure I get what you're saying. They're not really global right? They're namespaced under window.wp. If anything, import { keycodes } from 'utils'; is worse because it's not namespaced to WP core. By exporting to 'utils', you kind of make it global again, it's just a different context.

[ellatrix]

Even worse, in addition to plugins that can conflict with it, there's also an entire npm repository, and more. :) I think we should always export to something like wp/components etc.

[aduth]

I dislike the name "utils" because it's way too tempting a dumping ground for miscellany in lieu of better organization or naming. I'm speaking generally more to this specific case. This one is tricky, as is often the case. If I were to suggest anything, maybe something on the basis of events, keyboard, formatting, or constants (last being my pick). Or maybe we don't need it at all.

I don't feel particularly strongly about this, just thinking through the precedent we set. Maybe it's inevitable/preferable we have this folder than stress to be overly imaginative with our groupings (example).

To modularity point, I agree we'll need something to distinguish WordPress dependencies from others.

[nylen]

I chose utils partly because WP already contains a wp.utils. I agree with the concerns about it becoming a dumping ground; however, I would also like to avoid adding a ton of entry points.