Implement webpack hot module reloading. #4121

abotteram · 2017-12-21T12:18:36Z

Description

This PR implements webpack hot module reloading (HMR) for the editor.

Currently, the hot reloading works only for the actual editor itself and not other parts that are exposed through global variables such as blocks. I will be looking into how to do that at a later stage.

How Has This Been Tested?

I've tested this mainly by editing react components that are being used by the actual editor in /editor/index.js.

Screenshots (jpeg or gifs if applicable):

Types of changes

New development feature.

Checklist:

My code is tested.
My code follows the WordPress code style.
[] My code follows has proper inline documentation.

…g into try/implement-webpack-hmr

andreiglingeanu · 2017-12-22T14:03:38Z

That is amazing!

aduth · 2018-01-02T20:07:01Z

CONTRIBUTING.md

+define( 'GUTENBERG_WEBPACK_HMR', true );
+```
+
+Build the project ( `npm run build` ), and start the hmr development server by running `npm run hot`. Any changes to files used by the editor ( `/editor` ), should be reflected in the browser without reloading the page.


Should it be a requirement to npm run build first, or can this be part of the hot script?

Should HMR be built into npm run dev ?

At first I was confused into thinking /editor was the URL path, not the relative path from the repository. Not sure I have any suggestions here, aside from either assigning as a link to the absolute repository path or prefixing to clarify relative filesystem path (./editor)

aduth · 2018-01-02T20:08:04Z

CONTRIBUTING.md

+If the Webpack server is running on a different port than 3000, you need to configure Gutenberg to refer to the correct port. You can do this using the `GUTENBERG_WEBPACK_DEV_SERVER` constant:
+
+```php
+define( 'GUTENBERG_WEBPACK_DEV_SERVER', 'http://localhost:[your-port]' );


Could we overload the GUTENBERG_WEBPACK_HMR into supporting the default and overriding?

i.e.

<?php // These are the same: define( 'GUTENBERG_WEBPACK_HMR', true ); define( 'GUTENBERG_WEBPACK_HMR', 'http://localhost:3000' );

aduth · 2018-01-02T20:08:45Z

CONTRIBUTING.md

+define( 'GUTENBERG_WEBPACK_DEV_SERVER', 'http://localhost:[your-port]' );
+```
+
+*Warning*: This is still an experimental development feature.


I foresee this note lingering long past when it should, and would discourage introducing it in the first place.

aduth · 2018-01-02T20:09:44Z

blocks/api/registration.js

-		);
-		return;
+		if ( module.hot ) {
+			delete( blocks[ name ] );


delete is a keyword, not a function, and while I expect this should work as-is, the syntax could lead to confusion. I'd suggest:

delete blocks[ name ];

aduth · 2018-01-02T20:10:39Z

blocks/api/registration.js

-			'Block "' + name + '" is already registered.'
-		);
-		return;
+		if ( module.hot ) {


I'm not a huge fan of any code aside from the entry point file being HMR-aware, but I suppose this is a consequence of us having implemented the block registry as a shared singleton?

aduth · 2018-01-02T20:13:32Z

editor/index.js

@@ -46,6 +46,15 @@ window.jQuery( document ).on( 'heartbeat-tick', ( event, response ) => {
 	}
 } );

+const render = ( Component, props, target ) => {


This file is starting to get muddied with various rendering implementations. I'd suggest:

Create a DocBlock explaining the purpose of this function

Maybe rename as renderEditor, so we can avoid import assignment of elRender, and assume the first argument is the Editor component (since it's not used any other way).

aduth · 2018-01-02T20:14:55Z

lib/client-assets.php

 		array( 'wp-element', 'wp-components', 'wp-utils', 'wp-hooks', 'wp-i18n', 'tinymce-latest', 'tinymce-latest-lists', 'tinymce-latest-paste', 'tinymce-latest-table', 'media-views', 'media-models', 'shortcode' ),
-		filemtime( gutenberg_dir_path() . 'blocks/build/index.js' )
+        GUTENBERG_WEBPACK_HMR ? false : filemtime( gutenberg_dir_path() . 'blocks/build/index.js' )


Mixed whitespace, this should be tabbed, not spaced.

aduth · 2018-01-02T20:16:28Z

lib/client-assets.php

@@ -146,10 +157,11 @@ function gutenberg_register_scripts_and_styles() {
 	);
 	wp_register_script(
 		'wp-blocks',
-		gutenberg_url( 'blocks/build/index.js' ),
+		gutenberg_url( 'blocks/build/index.js', GUTENBERG_WEBPACK_HMR  ),


It's a bit odd to me that we pass the GUTENBERG_WEBPACK_HMR constant value in as an argument, as though we'd want to treat the function as generic and unaware of the constants, but then proceed to use GUTENBERG_WEBPACK_DEV_SERVER within the function implementation. I'd suggest going consistent one way or the other (either passing both as arguments, or referencing both within the function).

aduth · 2018-01-02T20:17:40Z

webpack.config.hot.js

+	const entryPointName = entryPointNames[ i ];
+	config.entry[ entryPointName ] = [ ...hrEntries, config.entry[ entryPointName ] ];
+}
+config.entry.hooks = [ config.entry.hooks ];


~~Can you explain what this line is doing?~~ Nevermind, I understand it now to be exempting hooks. Can you then explain why we need to exempt hooks?

aduth

package-lock.json needs to be updated. Are you running a version of npm lower than 5 ? (npm --version)

atimmer · 2018-01-03T18:34:01Z

This is a lot of work and hard to make sure it keeps working as new things are added. We've decided to not pursue this any further. For context see this chat: https://wordpress.slack.com/archives/C02QB2JS7/p1515003858000212. If anyone else wants to take a stab at it, this code is all yours.

Alexander Botteram and others added 9 commits December 21, 2017 10:40

Initial Webpack HMR support for the editor

2f04900

Prefixed defined constants with GUTENBERG_

53ecf95

Fixed broken constants

d9225ef

Fixed mistake in CONTRIBUTING.md

50ab6ab

Fixed PHPCS issues

f62362a

Fix notice about constant already being defined.

981a587

Merge branch 'try/implement-webpack-hmr' of github.com:Yoast/gutenber…

22e60ea

…g into try/implement-webpack-hmr

Add docs about the GUTENBERG_WEBPACK_DEV_SERVER constant.

aeeace4

Add HOT reloading for @wordpress/blocks

5f4c88f

aduth reviewed Jan 2, 2018

View reviewed changes

aduth requested changes Jan 2, 2018

View reviewed changes

atimmer closed this Jan 3, 2018

cliffordp mentioned this pull request Mar 19, 2020

Scripts: Replace live reload with React Fast Refresh in the start command #21008

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement webpack hot module reloading. #4121

Implement webpack hot module reloading. #4121

abotteram commented Dec 21, 2017

andreiglingeanu commented Dec 22, 2017

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth Jan 2, 2018

aduth left a comment

atimmer commented Jan 3, 2018

Implement webpack hot module reloading. #4121

Implement webpack hot module reloading. #4121

Conversation

abotteram commented Dec 21, 2017

Description

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

andreiglingeanu commented Dec 22, 2017

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 left a comment

Choose a reason for hiding this comment

atimmer commented Jan 3, 2018