-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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.
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
docs(guides): rewrite of development.md #1405
Conversation
Haven't worked with Re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I requested a lot of changes but mostly just in regards to semantics -- I'll give some thought to your questions/concerns and touch base with you after you've addressed this first pass. All in all, I think it's coming out great 👍
content/guides/development.md
Outdated
--- | ||
|
||
On this page we'll explain how to get started with developing and how to choose one of three tools to develop. It is assumed you already have a webpack configuration file. | ||
T> This guide uses examples from the [`Webpack Guides Code Examples`](https://github.com/TheDutchCoder/webpack-guides-code-examples) repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe hold off on mentioning this yet -- but definitely mention which guide is the prerequisite for this one if there is one.
content/guides/development.md
Outdated
|
||
W> The tools in this guide are meant for development **only**, do not ever use them in production! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might sound better as:
The tools in this guide are __only meant for development___, please __avoid__ using them in production!
content/guides/development.md
Outdated
|
||
Each editor has a different way of disabling this. For the most common ones: | ||
When webpack creates bundles from your code, it can become difficult to track down errors and warnings in your JavaScript. For example, if you bundle three source files (`a.js`, `b.js`, and `c.js`) into one bundle (`bundle.js`) and one of the source files contains an error, you will see the error in your console coming from `bundle.js` which isn't always very helpful (you want to know which source file the error came from). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rephrase this as:
When webpack bundles your source code, it can become difficult to track down errors and warnings to their original location. For example, if you bundle three source files (
a.js
,b.js
, andc.js
) into one bundle (bundle.js
) and one of the source files contains an error, the stack trace will simply point tobundle.js
. This isn't always very helpful as you probably want to know exactly which source file the error came from.
content/guides/development.md
Outdated
* **IntelliJ** - use search in the preferences to find "safe write" and disable it. | ||
* **Vim** - add `:set backupcopy=yes` in your settings. | ||
* **WebStorm** - uncheck `Use "safe write"` in Preferences > Appearance & Behavior > System Settings | ||
In order to make it easier to track down errors and warnings, JavaScript offers Source Maps, which maps your compiled code back to your original source code. So if an error originates from `b.js`, the Source Map will tell you exactly that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/s/So if/If
content/guides/development.md
Outdated
|
||
## Source Maps | ||
For this guide, let's use the `inline-source-map` option, which is good for illustrative purposes(but don't use it in production): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing a space between purposes
and the (
but I would also rephrase slightly:
For this guide, let's use the
inline-source-map
option, which is good for illustrative purposes (though not for production):
content/guides/development.md
Outdated
``` | ||
|
||
After installing, you can use the middleware like this: | ||
Now we can run `npm run server` from the command line and we will see our browser automatically loading up a new page (usually `localhost:8080`). If you now change any of the source files and save them, the web server will automatically reload after the code has been compiled. Give it a try! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"usually localhost:8080
"? Isn't it always localhost:8080?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, it isn't in my case as I already run a dev server there and have to change the port, but it's 8080 out of the box of course. Will change! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless a port is specified, webpack-dev-server runs portfinder to find the first open port starting at 8080: https://github.com/webpack/webpack-dev-server/blob/140da4573c7d36771d2dae3e0925f66394b27522/bin/webpack-dev-server.js#L347-L362
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, it would consistently fail on 8080 for me because there's my nginx proxy we use for docker.
Might be my setup though, so I took it out of the doc regardless :)
Thanks for explaining!
content/guides/development.md
Outdated
|
||
var app = express(); | ||
var compiler = webpack(webpackConfig); | ||
T> Now that your server is working, you might want to give [`Hot Module Replacement`](/guides/hot-module-replacement) a try. It's a logical next step! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave off the last bit and just end the first sentence with the exclamation point, e.g.
... a try!
content/guides/development.md
Outdated
|
||
|
||
## References | ||
## Conclusion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friggin auto formatting 😄
content/guides/development.md
Outdated
|
||
* [SurviveJS - Automatic Browser Refresh](http://survivejs.com/webpack/developing-with-webpack/automatic-browser-refresh/) | ||
* [webpack your Chrome DevTools Workspaces](https://medium.com/@rafaelideleon/webpack-your-chrome-devtools-workspaces-cb9cca8d50da) | ||
Now that you've learned how to automatically compile your code whenever you save a file, and how to run a simple development server, you can check out the next guide that covers [`Hot Module Replacement`](/guides/hot-module-replacement). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can leave out the second "how to" in this sentence and probably the "whenever you save a file bit" as well...
Now that you've learned how to automatically compile your code and run a simple development server, you can check out...
content/guides/development.md
Outdated
|
||
Depending on what you've used in `output.publicPath` and `output.filename`, your bundle should now be available on `http://localhost:3000/bundle.js`. | ||
W> This section is currently not working out of the box. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me give this some thought. webpack-dev-middleware
is what webpack-dev-server
uses under the hood so it could just be mentioned in that respect but a small example probably wouldn't be bad either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I like an example here, as people might have a need for middleware.
I'll take another stab at it later (maybe it was my node version again)
Love all the feedback! Will update tomorrow morning! |
Hm, something must have gone wrong in my merge. Do you want me to do an interactive rebase for this @skipjack ? |
@TheDutchCoder don't worry about it, we'll "Squash and Merge" this sucker when it's done. |
@TheDutchCoder seeing a couple of minor markdown formatting errors from the CI process: ./content/guides/development.md: 249: MD006 Consider starting bulleted lists at the beginning of the line
./content/guides/development.md: 249: MD007 Unordered list indentation
./content/guides/development.md: 86: MD040 Fenced code blocks should have a language specified Once those are fixed I can give this another quick review and hopefully ship it! |
Ok it looks like there's just two more things from content/guides/development.md:20:346: weasel_words.very Substitute 'damn' every time you're inclined to write 'very;' your editor will delete it and the writing will be just as it should be.
content/guides/development.md:156:24: cliches.write_good 'keep an eye on' is a cliché. The "very" should be removed but "keep an eye on" we could add an exception for. I'll do one last review now so we can get this merged soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor requests, aside from those this is looking really, really 🎉 .
content/guides/development.md
Outdated
* **IntelliJ** - use search in the preferences to find "safe write" and disable it. | ||
* **Vim** - add `:set backupcopy=yes` in your settings. | ||
* **WebStorm** - uncheck `Use "safe write"` in Preferences > Appearance & Behavior > System Settings | ||
In order to make it easier to track down errors and warnings, JavaScript offers Source Maps, which maps your compiled code back to your original source code. If an error originates from `b.js`, the Source Map will tell you exactly that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe link to somewhere else for an intro to source maps and rephrase slightly:
In order to make it easier to track down errors and warnings, JavaScript offers [source maps](http://blog.teamtreehouse.com/introduction-source-maps), which map your compiled code back to your original source code. If an error originates from `b.js`, the mapping will direct you to that file.
I would also lowercase "Source Maps" => "source maps" everywhere (except in titles).
content/guides/development.md
Outdated
to continue editing and saving your changes from Chrome or source files. | ||
1. webpack's Watch Mode | ||
2. webpack-dev-server | ||
3. webpack-dev-middlware |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
webpack-dev-middlware
=> webpack-dev-middleware
content/guides/development.md
Outdated
|
||
var app = express(); | ||
var compiler = webpack(webpackConfig); | ||
T> Now that your server is working, you might want to give [`Hot Module Replacement`](/guides/hot-module-replacement) a try! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take "Hot Module Replacement" out of the backticks (``) as it's not code.
content/guides/development.md
Outdated
|
||
Depending on what you've used in `output.publicPath` and `output.filename`, your bundle should now be available on `http://localhost:3000/bundle.js`. | ||
?> This section is currently not working out of the box. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe update this as a call to action? e.g.
?> Familiar with `webpack-dev-middleware`? We need your help! Please submit a PR to fill in the missing instructions and example here. Make sure to keep it simple as this guide is intended for beginners.
@skipjack cool, thanks for all the help, just pushed the latest changes, so ready for a (hopefully) final check! |
This PR contains a rewrite of the
Development
guide.There are a couple of issues we need to resolve before considering a merge:
Watch Mode with Chrome DevTools Workspaces
has been removed as I found it oddly specific, but we might want to include it somewhere else or rework it a little bit. Thoughts @skipjack ?webpack-dev-middleware
does not work out of the box at all (throws all kinds offs
errors). I'm too green to figure this out (adding the{node: fs: 'empty' }
solution doesn't work either, not the{target: 'node'}
one. Help is very welcome!All feedback appreciated!