Skip to content
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

Initial Guides Cleanup and Review #1273

Merged
merged 9 commits into from
Jun 12, 2017
Merged

Initial Guides Cleanup and Review #1273

merged 9 commits into from
Jun 12, 2017

Conversation

skipjack
Copy link
Collaborator

@skipjack skipjack commented Jun 7, 2017

@TheDutchCoder ok pretty much have this wrapped up -- just need to do a bit more sorting based on our previous conversation. The main change to getting-started was to clarify the directory structure, use /src instead of /app (as I think it's a bit more common), and consistently use /dist to house the entire production site (e.g. index.html as well). The last part will come into play specifically in our output-management guide but probably in a few others as well.

Look through the changes and let me know what you think. I think it'll give us a nice base for simplifying and updating the other guides. I'll ping a few others for review as well once we've agreed it's good to go.

@TheDutchCoder
Copy link
Collaborator

@skipjack This looks like a fantastic start! Great work!

We might tweak the language here and there of course, but that's all part of the WIP.
Looks good to me!

@skipjack skipjack requested review from simon04 and bebraw June 8, 2017 12:48
@skipjack
Copy link
Collaborator Author

skipjack commented Jun 8, 2017

@TheDutchCoder ok great... just finished sorting them, I tried to push everything that's either incomplete or a WIP to the bottom while sorting the more complete guides in an order that, at least somewhat, makes for a nice progression. Feel free to review and add any comments as you see fit. I think the next step will be coming up with a plan to divide and conquer reviewing, simplifying, and synchronizing the rest with getting-started.

@bebraw @simon04 can one (or both) of you give this a quick review as well?


## Basic Setup

First let's create a directory, initialize NPM, and [install webpack locally](/guides/installation#local-installation):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's npm I think. They have something on this in their branding guidelines.


## NPM Scripts

Given it's not particularly fun to run a local copy of webpack from the CLI, we can set up a little shortcut. Let's adjust our _package.json_ by adding an [NPM script]():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NPM -> npm. Also missing a link.

```


Now the `npm run build` command can be used in place of the longer commands we used earlier. Also note that within `scripts` we can reference local `node_modules` by name instead writing out the entire path. This convention is the standard in most npm-based projects.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Also" feels redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "Now the npm run build command can be used in place of the longer commands we used earlier. Note that within scripts we can reference locally installed npm packages by name instead of writing out the entire path. This convention is the standard in most npm-based projects and allows us to directly call webpack, instead of node_modules/webpack/bin/webpack.js"

@@ -185,7 +185,7 @@ module.exports = function(env) {
};
```

With the above webpack config, we see three bundles being generated. `vendor`, `main` and `manifest` bundles.
With the above webpack config, we see three bundles being generated. `vendor`, `main` and `manifest` bundles.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"With the above webpack config, we see three bundles being generated: vendor, main and manifest."
Might read a little nicer.

function component () {
var element = document.createElement('div');

/* lodash is required for the next line to work */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/* lodash is required for the next line to work, we will add lodash to the index.html file for now */
Or something along those lines, so people know where lodash is "coming from"?

- If a dependency is missing, or included in the wrong order, the application will not function properly.
- If a dependency is included but not used, the browser will be forced to download unnecessary code.

Let's try using webpack to manage these scripts instead...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "Let's use webpack to manage these scripts instead."


## Creating a Bundle

First we'll tweak our directory structure slightly, separating the "source" code we write and edit (`/src`) from our "distribution" code (`/dist`) which is actually shipped to the browser:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "First we'll tweak our directory structure slightly, separating the "source" code (/src) from our "distribution" code (/dist). The "source" code is the code we will write and edit. The "distribution" code is the code that will eventually be loaded in the browser:

npm install --save lodash
```

and then import it from our script...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "and then import it in our script..."

// ...
```

We also need to change our update our `<script>` tags to point to the unified bundle:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "Since we now start bundling our scripts, we need to update our <script> tag to load the bundle, instead of individual files. We should also remove the <script> tag that loads lodash, as we now import lodash in our script with webpack:"


## ES2015 Modules

Notice the use of [ES2015 module `import`](https://developer.mozilla.org//en-US/docs/Web/JavaScript/Reference/Statements/import) (alias ES2015, *harmony*) in `src/index.js`? Although `import`/`export` statements are not supported in browsers (yet), webpack supports them and will replace those instructions with ES5 compatible wrapper code in the output. Inspect `dist/bundle.js` if you need to assure yourself.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might get a little technical for beginners, maybe we can word it a little friendlier?

Suggestion: "Although import/export statements are not supported in most browsers (yet), webpack does support them. Behind the scenes, webpack actually "transpiles" the code so that older browsers can also run it. If you inspect dist/bundle.js, you might be able to see how webpack does this, it's quite ingenious!"


## Using a Configuration

For a more complex setup, we can use a webpack [configuration file](/concepts/configuration). Let's add this file and represent the CLI command used above with some basic configuration:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "You can also create a configuration file that webpack can use when you run webpack. This is much more efficient than having to type in a lot of commands in the terminal, so let's create one for our basic setup:"


Now the `npm run build` command can be used in place of the longer commands we used earlier. Also note that within `scripts` we can reference local `node_modules` by name instead writing out the entire path. This convention is the standard in most npm-based projects.

T> You can pass custom parameters to webpack by adding two dashes to the `npm run build` command, e.g. `npm run build -- --colors`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "You can pass custom parameters to webpack by adding two dashes between the npm run build command and your parameters, e.g. npm run build -- --colors"


## Conclusion

Now that you have a basic build together, you should dig into the [basic concepts](/concepts) and [configuration](/configuration) to better understand webpack's design. The [API](/api) section digs into the various interfaces webpack offers. Or, if you'd prefer learning by example, select the next guide from the list and continue building out this little demo we've been working on -- which, if you've been paying attention, should now look like this:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: remove the '--'


### Local Installation
Before we begin, make sure you have a fresh version of [Node.js](https://nodejs.org/en/) installed. The current LTS is an ideal starting point. You may run into a variety of issues with the older versions as they may be missing functionality webpack and/or its related packages require.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: explain what LTS stand for.

@skipjack
Copy link
Collaborator Author

skipjack commented Jun 8, 2017

@TheDutchCoder @bebraw thanks for the thorough review -- I'll address these things tonight.

@skipjack
Copy link
Collaborator Author

skipjack commented Jun 9, 2017

Ok good for a final review! I resolved most things word for word as suggested with a few exceptions where I tweaked the wording a bit.

@TheDutchCoder
Copy link
Collaborator

This is great stuff @skipjack love the additions you made!

Copy link
Collaborator

@TheDutchCoder TheDutchCoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@webpack webpack deleted a comment from jsf-clabot Jun 10, 2017
@skipjack skipjack mentioned this pull request Jun 10, 2017
15 tasks
@skipjack skipjack merged commit cadfa9e into master Jun 12, 2017
@skipjack skipjack deleted the guides-cleanup branch June 12, 2017 13:52
@skipjack
Copy link
Collaborator Author

Ok finally merged as all links were fixed except for the "edit" links which were expected to fail as the names of those pages change. If anything comes up with markdown linting etc, I'll address in a separate PR.

@skipjack
Copy link
Collaborator Author

@TheDutchCoder with this merged I think we can start to divide and conquer the rest. Are there any in particular you'd like to tackle? I know you already have that one open for Code Splitting so maybe those three would be a good start. It would be great if we could simplify those to the point where one long article would work. I think I'd like to take "Lazy Load - React" and turn it into to just "Lazy Loading" but aside from that I'm open to whichever.

@TheDutchCoder
Copy link
Collaborator

Hey @skipjack sorry I've been a bit busy, but I will make some time on the weekend to go through all this and decide which topics I'd like to cover. I'm okay with anything but, since I'm a beginner, the earlier topics might be best suited for me ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants