HMR guide rewrite #1439

sbaidon · 2017-07-17T15:08:16Z

This PR contains some changes to the HMR guide:

Update examples to be consistent with Development guide

Initial Commit

align examples to previous guides

skipjack

Thanks for tackling this @sbaidon, this is a great start! Sorry for the delayed review -- I've been busy the last few days. I added a few comments and I'd like to let @TheDutchCoder take a look as well.

skipjack · 2017-07-24T21:42:30Z

content/guides/hot-module-replacement.md

-    contentBase: path.resolve(__dirname, 'dist'),
-    publicPath: '/'
+    filename: '[name].bundle.js',
+    path: path.resolve(__dirname, 'dist')
  }
 };


Looks like the indentation is a bit off in the bottom section (i.e. from the closing plugins array bracket through the final closing bracket of the whole configuration object). Note that for all our guide diffs we use 2 space indentation as a base so we can squeeze the + and - signs in.

Yeah just use 2 spaces for indentation and 2 spaces at the start of each line in a diff block for the + and -.

skipjack · 2017-07-24T22:00:11Z

content/guides/hot-module-replacement.md

-Not too bad, huh? Let's test it out using `module.hot.accept`...
+You can also use the CLI to modify the [webpack-dev-server](https://github.com/webpack/webpack-dev-server) configuration with the following command: `webpack-dev-server --hotOnly`.
+
+Now let's update the `index.js` file so that when a change inside `print.js` is detected we tell webpack to accept the updated module.


I think you need to mention that the reader should npm start their webpack-dev-server before altering the print.js module.

skipjack · 2017-07-24T22:01:08Z

content/guides/hot-module-replacement.md

+  return element;
+}
+
+document.body.appendChild(component());
 ```


You might be able to just leave out this whole section as nothing has changed from the development guide.

I did add the module.hot part, or do you mean doing something like this:

import _ from 'lodash'; import printMe from './print.js'; if (module.hot) { module.hot.accept('./print.js', function() { console.log('Accepting the updated printMe module!'); printMe(); }) } ...

No what you did was fine IMO, just make it a full diff ;)

skipjack · 2017-07-24T22:02:38Z

content/guides/hot-module-replacement.md

-Hot Module Replacement can be tricky. For example, let's say I have the following class:
+Hot Module Replacement can be tricky. To show this, let's go back to our working example. If you go ahead and click the button on the example page, you will realize the console is printing the old `printMe` function.
+
+This is happening because the button's `onclick` event handler is still bind to the original `printMe` function.


/s/still bind/still bound

skipjack · 2017-07-24T22:04:05Z

content/guides/hot-module-replacement.md

+    module.hot.accept('./print.js', function() {
+    console.log('Accepting the updated printMe module!');
+    document.body.removeChild(element);
+    element = component(); // update binding now that new printMe module has been acepted


I would rewrite the comment as:

Re-render the "component" to update the click handler.

skipjack · 2017-07-24T22:10:40Z

content/guides/hot-module-replacement.md

+import printMe from './print.js';
+
+if (module.hot) {
+    module.hot.accept('./print.js', function() {


This line is indented two tabs instead of one. However, I think this whole block should be changed to a diff and indented two extra lines.

skipjack · 2017-07-24T22:11:42Z

content/guides/hot-module-replacement.md

 ```

-


The extra line break is intentional -- we usually try to keep some spacing between sections (especially for h2s).

skipjack · 2017-07-24T22:12:37Z

content/guides/hot-module-replacement.md

+```
+
+This is just one example, but there are many others that can easily trip people up. Luckily, there are a lot of loaders out there, some mentioned below, that will make using this process much easier.
+


Add an extra line break between sections (similar to my other h2 comment above).

skipjack · 2017-07-24T22:14:00Z

content/guides/hot-module-replacement.md

+
+Hot Module Replacement with CSS is actually fairly straightforward with the help of the `style-loader`. This loader uses `module.hot.accept` behind the scenes to patch `<style>` tags when CSS dependencies are updated.
+
+Now let's update the configuration file to make use of the loader.


You should probably add a quick installation line before the config changes, e.g.

npm install --save-dev style-loader css-loader

Also below the installation line you should put the name of the file that's being edited for consistency with the other guides e.g.

webpack.config.js

...

I actually thought about this, but I noticed that on the previous examples style-loader and css-loader are already added as dev-dependencies, should I write this just as a reminder in case they haven't been installed?

I think it's only mentioned in asset-management which is then cleaned up at the end. I would mention it again just as a reminder.

I agree, it's also best to make sure that people who don't follow all the guides in succession are still able to fairly easily follow along with this particular guide.

I'd rather be a bit too verbose in these cases.

skipjack · 2017-07-24T22:17:08Z

content/guides/hot-module-replacement.md

@@ -152,7 +222,6 @@ body {

 Change the style on `body` to `background: red;` and you should immediately see the page's background color change without a full refresh.

-


Same thing re line breaks.

sbaidon · 2017-07-24T22:26:50Z

Thanks for the review will submit changes ASAP!

TheDutchCoder · 2017-07-24T22:44:10Z

content/guides/hot-module-replacement.md


 __index.js__

 ``` js
-import Library from './library';


make this a diff and use the +and - for consistency :)

TheDutchCoder · 2017-07-24T22:46:58Z

content/guides/hot-module-replacement.md

@@ -18,125 +19,194 @@ related:
    url: /api/hot-module-replacement
 ---

+T> This guide extends on code examples found in the [`Development`](/guides/development) guide.


Can you remove the backticks from "[Development]"? We use them for code, but decided to not use them for links in the guides.

TheDutchCoder · 2017-07-24T22:51:31Z

content/guides/hot-module-replacement.md

+document.body.appendChild(element);
+```
+
+This is just one example, but there are many others that can easily trip people up. Luckily, there are a lot of loaders out there, some mentioned below, that will make using this process much easier.


I would suggest to change this slightly for a nicer sentence:

This is just one example, but there are many others that can easily trip people up. Luckily, there are a lot of loaders out there (some of which are mentioned below) that will make hot module replacement much easier.

TheDutchCoder · 2017-07-24T22:54:25Z

content/guides/hot-module-replacement.md

+      filename: '[name].bundle.js',
+      path: path.resolve(__dirname, 'dist')
+    }
+  };
 ```

 hot loading stylesheets is a breeze...


I would change this slightly to keep the tone a little more friendly, maybe:

Hot loading stylesheets is as easy as importing them into a module:

TheDutchCoder · 2017-07-24T22:54:31Z

content/guides/hot-module-replacement.md

+      filename: '[name].bundle.js',
+      path: path.resolve(__dirname, 'dist')
+    }
+  };
 ```

 hot loading stylesheets is a breeze...

 __index.js__

 ``` js


Make this a diff and include the full file for consistency (I know it's a bit verbose in this case, but we can always later on decide to only include the actual diff-ed sections).

Can you also add in a diff for the project structure (you're adding a new file, right?), and put the new file contents in a code block as well. Unless the file is already present, then a diff is sufficient (but I think it's not there anymore, as one of the guides does a cleanup at the end).

TheDutchCoder · 2017-07-24T22:55:35Z

content/guides/hot-module-replacement.md

@@ -152,7 +222,6 @@ body {

 Change the style on `body` to `background: red;` and you should immediately see the page's background color change without a full refresh.


Would also include a diff here for the stylesheet instead of describing what to to.

E.g.

body { - background: green; + background: red; }

TheDutchCoder

Looks really good so far, thanks for all the hard work!

I just added some additional comments to check out.

Don't worry if not all of them make sense, I'm always willing to commit a few changes in the end as well!

sbaidon · 2017-07-25T14:34:01Z

Just pushed the changes

Hope I didn't miss anything!

evolve2k · 2017-07-26T12:55:17Z

I today opened a PR to request ehnancements to this documentation, @TheDutchCoder just kindly redirected me here. Glad to see this rewrite in progress.
I've had a quick look over the commit history but it's hard to tell at a glance if my thoughts have been addressed.. So here's what I said earlier. If they are already addressed that's great! If not please feel free to account for them.

Feedback on current live documentation

Progressing through the tutorial so far, each tute lead on nicely to the next.
This one however breaks the flow and provided no clues as to which lines to add like previous files do.

Basically the convention of the green lines beginning with + signs is not present in this file, breaking the flow of the tutorial at this point.

TheDutchCoder · 2017-07-26T12:58:28Z

@evolve2k Yeah we're doing a major guides overhaul. Some of them are "done" and in place, but a couple of them are still WIPs and don't follow the same "flow" as the ones that are done.

It's our goal to make each guide smoothly follow up on a previous one.

If you find any of them lacking, please let us know (and even open a PR if you like), we're always happy with contributions!

TheDutchCoder

Just one small change, rest is looking really good so far! 💯

TheDutchCoder · 2017-07-26T13:01:31Z

content/guides/hot-module-replacement.md

 ```

 __styles.css__

 ``` css
 body {
-  background: blue;
+  background: red;


This should be background: blue; to make it line up with the diff further down ;)

skipjack · 2017-07-26T13:13:58Z

@evolve2k yeah you can check out this comment on #1258 for more information on the current guides work. I've been keeping it updated based on the work and PRs submitted.

skipjack

This is looking great! Added a few last minor comments... once those and @TheDutchCoder's are addressed this should be good to go 👍

skipjack · 2017-07-26T13:15:19Z

content/guides/hot-module-replacement.md

-  }
-};
-```
+To get it up and running let's do `npm start` from the command line.


I would rephrase "let's do..." => "let's run..."

skipjack · 2017-07-26T13:17:43Z

content/guides/hot-module-replacement.md

-class Logger {
-  log(text) {
-    console.log('Logging some text: ', text)
+To make this work with HMR we need to update that binding to the new `printMe` function using `module.hot.accept`


You should probably to add some punctuation to the end of this sentence (e.g. :).

skipjack · 2017-07-26T13:19:14Z

content/guides/hot-module-replacement.md

+
+Hot Module Replacement with CSS is actually fairly straightforward with the help of the `style-loader`. This loader uses `module.hot.accept` behind the scenes to patch `<style>` tags when CSS dependencies are updated.
+
+First let's install both loaders with the following command: `npm install --save-dev style-loader css-loader`


You should break the command to a new line e.g.

First let's install both loaders with the following command: ``` bash npm install --save-dev style-loader css-loader ```

sbaidon · 2017-07-26T13:39:21Z

Just made the changes, hope everything is alright now 👍

TheDutchCoder

💥 Awesome!

skipjack · 2017-07-26T13:51:42Z

@TheDutchCoder I'll leave this to you to "Squash and Merge" once the build checks finish.

TheDutchCoder · 2017-07-26T14:06:44Z

Yes sir, will do!

TheDutchCoder · 2017-07-26T15:11:57Z

@skipjack was that a better "Squash & Merge"? :)

sbaidon · 2017-07-26T16:07:19Z

I just noticed I was missing a lodash import in a diff block, I opened up a new PR #1456 is that ok?

skipjack · 2017-07-26T16:30:59Z

@TheDutchCoder beautiful 🚀 🎉 😆 ... feel free to also drop the bullet points from the body, e.g.

docs(guides): rewrite of the HMR guide (#1439)

Rewrite of the hot-module-replacement guide.

@sbaidon that's perfect, thanks for following up!

TheDutchCoder · 2017-07-26T16:31:15Z

Yeah no problem! On Jul 26, 2017 12:07 PM, "Sergio Emir Baidon Carrillo" < [email protected]> wrote: I just noticed I was missing a lodash import in a diff block, I opened up a new PR #1456 <#1456> is that ok? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1439 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABl0bwg1_RTe2-2YHvY2rhlqSL45fK4sks5sR2Q4gaJpZM4OaHID> .

sbaidon added 2 commits July 17, 2017 09:39

Rewrite hot-module-replacement.md

e7cbebc

Initial Commit

First changes

6e03643

align examples to previous guides

sbaidon mentioned this pull request Jul 17, 2017

Guides - Review and Simplify #1258

Closed

skipjack added the Guides label Jul 17, 2017

skipjack mentioned this pull request Jul 18, 2017

Guides - Issue in HMR Guide #1443

Closed

sbaidon added 2 commits July 18, 2017 11:13

Merge branch 'master' into hmr-guide-rewrite

ece4867

Fix markdown lint issues

6d72787

skipjack self-requested a review July 18, 2017 20:55

skipjack mentioned this pull request Jul 19, 2017

chore(hmr): add initial hot-module-replacement example TheDutchCoder/webpack-guides-code-examples#18

Merged

sbaidon and others added 2 commits July 21, 2017 12:46

Merge branch 'master' into hmr-guide-rewrite

c50fedc

Merge branch 'master' into hmr-guide-rewrite

becabc3

skipjack requested a review from TheDutchCoder July 24, 2017 22:09

skipjack requested changes Jul 24, 2017

View reviewed changes

skipjack reviewed Jul 24, 2017

View reviewed changes

TheDutchCoder reviewed Jul 24, 2017

View reviewed changes

TheDutchCoder requested changes Jul 24, 2017

View reviewed changes

Update hot-module-replacement.md

e55509f

TheDutchCoder mentioned this pull request Jul 26, 2017

Update hot-module-replacement.md #1454

Closed

TheDutchCoder requested changes Jul 26, 2017

View reviewed changes

skipjack requested changes Jul 26, 2017

View reviewed changes

small fixes

ae4eca2

TheDutchCoder approved these changes Jul 26, 2017

View reviewed changes

skipjack approved these changes Jul 26, 2017

View reviewed changes

Merge branch 'master' into hmr-guide-rewrite

34fe362

TheDutchCoder closed this Jul 26, 2017

TheDutchCoder reopened this Jul 26, 2017

TheDutchCoder merged commit 595e0ac into webpack:master Jul 26, 2017

skipjack mentioned this pull request Jul 28, 2017

Guides - Warnings/Errors in Hot Module Replacement #1461

Closed

		```

		This is just one example, but there are many others that can easily trip people up. Luckily, there are a lot of loaders out there, some mentioned below, that will make using this process much easier.


		Hot Module Replacement with CSS is actually fairly straightforward with the help of the `style-loader`. This loader uses `module.hot.accept` behind the scenes to patch `<style>` tags when CSS dependencies are updated.

		Now let's update the configuration file to make use of the loader.

		@@ -152,7 +222,6 @@ body {

		Change the style on `body` to `background: red;` and you should immediately see the page's background color change without a full refresh.


		Hot Module Replacement with CSS is actually fairly straightforward with the help of the `style-loader`. This loader uses `module.hot.accept` behind the scenes to patch `<style>` tags when CSS dependencies are updated.

		First let's install both loaders with the following command: `npm install --save-dev style-loader css-loader`

HMR guide rewrite #1439

HMR guide rewrite #1439

Conversation

sbaidon commented Jul 17, 2017

skipjack 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

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

sbaidon commented Jul 24, 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

TheDutchCoder Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheDutchCoder Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

TheDutchCoder left a comment

Choose a reason for hiding this comment

sbaidon commented Jul 25, 2017

evolve2k commented Jul 26, 2017

Feedback on current live documentation

TheDutchCoder commented Jul 26, 2017

TheDutchCoder left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipjack commented Jul 26, 2017

skipjack 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

sbaidon commented Jul 26, 2017

TheDutchCoder left a comment

Choose a reason for hiding this comment

skipjack commented Jul 26, 2017

TheDutchCoder commented Jul 26, 2017

TheDutchCoder commented Jul 26, 2017

sbaidon commented Jul 26, 2017

skipjack commented Jul 26, 2017

TheDutchCoder commented Jul 26, 2017 via email

TheDutchCoder Jul 24, 2017 •

edited

Loading

TheDutchCoder Jul 24, 2017 •

edited

Loading

TheDutchCoder left a comment •

edited

Loading