-
-
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
Rework Code Splitting Guides #1338
Conversation
28d9eef
to
95f1595
Compare
Ok this is pretty much ready for review if anyone wants to start. I'm just working on the Dynamic Imports section of Code Splitting now. |
content/guides/code-splitting.md
Outdated
|
||
A typical application can depend on many third party libraries for framework/functionality needs. Unlike application code, code present in these libraries does not change often. | ||
__webpack.config.js__ |
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.
Would it be a good idea to continue with the same files we created in the earlier guides?
If you want I can create a PR after this is all merged to try and tie them together.
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 mean do the file diffs we did in other guides? Yeah we should add those here for consistency as well.
content/guides/code-splitting.md
Outdated
|
||
## On Demand Code Splitting | ||
The first of these two points is definitely an issue for our example, as `moment` is also imported within `./src/index.js` and will thus be duplicated in the output. See the `CommonChunkPlugin` example below for a solution to this problem. |
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.
Is moment
not lodash
? :)
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.
Yep, good catch.
content/guides/code-splitting.md
Outdated
|
||
Learn [how to split on demand](/guides/code-splitting-async) using `import()` or `require.ensure()`. | ||
There are multiple plugins and loaders in webpack's ecosystem that can help with splitting and managing split code. The most widely utilized of these is the [`CommonsChunkPlugin`](/plugins/commons-chunk-plugin), a fairly advanced tool that allows us to extract common dependencies into an existing entry chunk or an entirely new chunk. Let's use this to de-duplicate the `moment` dependency from the previous example: |
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.
lodash
?
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.
😆 ahh man... I'll correct this as well.
content/guides/code-splitting.md
Outdated
This will yield the following build result (notice how our `index.bundle.js` has shrunk in size): | ||
|
||
?> Add bash example of webpack output | ||
|
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.
Elaborate/Illustrate a bit on what the plugin has done under the hood, so that it's clear what happened.
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.
Will do 👍
content/guides/code-splitting.md
Outdated
|
||
Here are some other useful plugins and loaders provide by the community for splitting code: | ||
|
||
- [`ExtractTextPlugin`](/plugins/extract-text-webpack-plugin): Useful for splitting CSS out from the main application. |
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.
404 on this link
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.
Where are you seeing the 404? It works fine for me.
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.
Ah, it's a relative URL, so it will 404 when I view this file for example.
Probably not an issue in the build then!
Here are some other useful plugins and loaders provide by the community for splitting code: | ||
|
||
- [`ExtractTextPlugin`](/plugins/extract-text-webpack-plugin): Useful for splitting CSS out from the main application. | ||
- [`bundle-loader`](/loaders/bundle-loader): Used to split code and lazy load the resulting bundles. |
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.
404 on this link
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.
This one works fine for me as well.
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.
Plus the CI build/test didn't catch any issues with either of them (just the edit links which is ok).
content/guides/code-splitting.md
Outdated
|
||
Now, when we run webpack, we should see lodash separated out to a separate bundle: | ||
|
||
?> Add bash example of webpack output |
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.
Hash: 5f3b08906b5e7d8d0799
Version: webpack 2.6.1
Time: 527ms
Asset Size Chunks Chunk Names
index.bundle.js 542 bytes 0 [emitted] index
vendor.bundle.js 547 kB 1 [emitted] [big] vendor
[0] ./~/lodash/lodash.js 540 kB {1} [built]
[1] (webpack)/buildin/global.js 509 bytes {1} [built]
[2] (webpack)/buildin/module.js 517 bytes {1} [built]
[3] ./src/index.js 401 bytes {0} [built]
[4] multi lodash 28 bytes {1} [built]
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.
👍
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.
Btw, are you maintaining a repo somewhere to test all these things from the guides? Would love to help out there and use it for testing...
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 actually just do it all locally, doing step by step to see the results, but maybe it would be an idea to setup a repo with the actual sources per guide or something?
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 the more we go through this the more I think it wouldn't be a bad idea, at least for testing. Maybe at some point we expose it as an "official" boilerplate to follow along with the guides but for now should at least be useful to us.
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.
By the way I added this but at line 110
which is where I think you meant it to be. The example at 142
should yield a lodash.bundle.js
file although things might be a little confusing without the file/directory diffs in place yet.
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.
Okay let me double check this, maybe I added the wrong output :)
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.
Nope this results in index.bundle.js
and vendor.bundle.js
, not lodash.bundle.js
.
Ahhh, this is a webpack 2.4.0+ specific thing, I still run 2.6.1, so that's probably why.
We should probably make a note of which version to use for the guides. I'm assuming the latest stable build for the major version?
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.
@TheDutchCoder I believe the discrepancy is because I haven't added the file diff and I may have forgotten to diff the removal of the extra entry
and CommonsChunkPlugin
. The import()
behavior is smart enough not to split things out if the package has already been included elsewhere.
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'll review and update this section tonight to make it clearer.
content/guides/code-splitting.md
Outdated
__webpack.config.js__ | ||
|
||
``` js | ||
const path = require('path'); |
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.
We use var
everywhere else still.
I'm okay with using ES6, but it would be good to keep it consistent.
Adjust to var
for now?
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.
Are you saying we use var
specifically for require
d packages in the other guides' webpack.config.js examples? I'd rather use const
here and I'd be happy to go update the others for consistency.
If you're talking about var
/const
/let
usage in general I think it's ok to use them where they make sense as each are used for somewhat different things.
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.
Okay sounds good to me.
It's just that const/let is not implemented in all browsers yet and I think we shouldn't assume everyone uses babel for instance.
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, so yes you are correct that maybe within the /src
files we shouldn't but within the configs it should be ok because Node has supported it for a little while now. So I would revise my earlier statement to be Node imports within webpack.config.js
examples should be const name = require('package')
. That's what we're using in the rest of the docs as well and I think it's the "recommended" practice until Node supports import
and export
syntax.
content/guides/code-splitting.md
Outdated
__src/index.js__ | ||
|
||
``` diff | ||
async function getComponent() { |
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.
Don't forget to diff the 1st line ;)
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.
Ah good catch.
@@ -53,7 +53,11 @@ export default { | |||
|
|||
### `import()` | |||
|
|||
Dynamically load modules. | |||
`import('path/to/module') -> Promise` |
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.
Can we remove the -> Promise
part, as it looks like something that the user should actually type in their code.
Maybe something like:
import('path/to/module') // returns a Promise
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 actually copied this from one of the other guides and modified it slightly. It's supposed to be a method/function "signature" I think. @bebraw is there an agreed upon standard syntax for this somewhere? I feel like there's a mix throughout the documentation that should probably be made consistent at some point.
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.
Nope. It should be specified at the writer's guide.
content/api/module-methods.md
Outdated
The compiler treats this as a split point and will split everything from `lodash` into a separate bundle. This returns a promise that will resolve to the module once the bundle has been loaded. See the [async code splitting guide](/guides/code-splitting-async) for more information. | ||
W> This feature relies on [`Promise`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) internally. If you use `import()` with older browsers, remember to shim `Promise` using a polyfill such as [es6-promise](https://github.com/stefanpenner/es6-promise) or [promise-polyfill](https://github.com/taylorhakes/promise-polyfill). See [Shimming](/guides/shimming) for more information. | ||
|
||
One problem with direct spec-based usage is that we have no control over the split chunk's name or other properties. Luckily webpack allows some special parameters via comments so as to not break the spec: |
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.
"One problem with direct spec-based usage ..." This might be a bit difficult to understand for beginners. How about something like: "The spec for ìmport` doesn't allow control over the chunk's name or other properties ..."
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.
Yep, that sounds better thanks!
); | ||
``` | ||
|
||
`webpackChunkName`: A name for the new chunk. Since webpack 2.6.0, the placeholders `[index]` and `[request]` are supported within the given string to an incremented number or the actual resolved filename respectively. |
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 love to see an example + output for this as a beginner 😄
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.
There will be in both the lazy-loading
and code-splitting
guides. I want to avoid too many and too detailed examples in the actual reference documentation but we could potentially add a link to the guide here.
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 double-checked and it is used in both the Dynamic Imports example in Code Splitting as well as the example in Lazy Loading.
content/guides/getting-started.md
Outdated
@@ -109,7 +109,7 @@ __src/index.js__ | |||
+ | |||
function component() { | |||
var element = document.createElement('div'); | |||
|
|||
- // Lodash, currently included via a script, is required for this line to work |
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.
Can we leave this comment? It's to illustrate that in the basic example we explicitly depend on lodash.
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.
See my comment in #1337
content/guides/lazy-loading.md
Outdated
Let's take the example from [Code Splitting](/guides/code-splitting#dynamic-imports) and tweak it a bit to demonstrate this concept even more. The code there does cause a separate chunk, `lodash.bundle.js`, to be generated and technically "lazy-loads" it as soon as the script is run. The trouble is that no user interaction is required to load the bundle -- meaning that every time the page is loaded, the request will fire. This doesn't help us too much, and actually may impact performance negatively. | ||
|
||
Let's try something different. We'll add an interaction to log some text to the console when the user clicks a button. However, we'll wait to load that code until the actual interaction occurs for the first time. To do this we'll go back and extend the original example from [Getting Started](/guides/getting-started) and leave the `lodash` in the main chunk. | ||
|
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.
Can you add the project folder's structure in here?
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.
Yep, as discussed above, we'll make sure to add the file/directory diffs throughout both guides for consistency.
content/guides/lazy-loading.md
Outdated
function component() { | ||
var element = document.createElement('div'); | ||
+ var button = document.createElement('button'); | ||
+ var break = document.createElement('br'); |
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.
break
is a reserved word in a switch, better use br
or something (linters may not like it).
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.
Ah good catch, will do.
content/guides/lazy-loading.md
Outdated
+ element.appendChild(break); | ||
+ element.appendChild(button); | ||
+ | ||
+ button.onclick = e => import(/* webpackChunkName: "console" */ './console').then(module => { |
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.
Should be './print'
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.
👍
content/guides/lazy-loading.md
Outdated
|
||
Now let's run webpack and check out our new lazy-loading functionality: | ||
|
||
?> Add bash example of webpack output |
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.
This will work, but the index.html
file doesn't reference the right files. I think this will be solved in my guide, but just wanted to make a not of it.
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 think this will be solved in my guide, but just wanted to make a not of it.
Solved by the edits to the output-management
guide?
content/guides/code-splitting.md
Outdated
|
||
You might also want to split your styles into a separate bundle, independent from application logic. | ||
This enhances cacheability of your styles and allows the browser to load the styles in-parallel with your application code, thus preventing a FOUC ([flash of unstyled content](https://en.wikipedia.org/wiki/Flash_of_unstyled_content)). | ||
?> Add bash example of webpack output |
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.
Hash: d1e4eab63dc6ba09c930
Version: webpack 2.6.1
Time: 531ms
Asset Size Chunks Chunk Names
vendor.bundle.js 544 kB 0 [emitted] [big] vendor
index.bundle.js 544 kB 1 [emitted] [big] index
[0] ./~/lodash/lodash.js 540 kB {0} {1} [built]
[1] (webpack)/buildin/global.js 509 bytes {0} {1} [built]
[2] (webpack)/buildin/module.js 517 bytes {0} {1} [built]
[3] ./src/index.js 216 bytes {1} [built]
[4] multi lodash 28 bytes {0} [built]
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.
🎉
content/guides/code-splitting.md
Outdated
|
||
``` js | ||
const path = require('path'); | ||
const webpack = require('webpack'); |
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.
Make this a diff
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.
👍
content/guides/code-splitting.md
Outdated
|
||
This will yield the following build result (notice how our `index.bundle.js` has shrunk in size): | ||
|
||
?> Add bash example of webpack output |
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.
Hash: bc0cb296200698aee722
Version: webpack 2.6.1
Time: 540ms
Asset Size Chunks Chunk Names
index.bundle.js 665 bytes 0 [emitted] index
vendor.bundle.js 547 kB 1 [emitted] [big] vendor
[0] ./~/lodash/lodash.js 540 kB {1} [built]
[1] (webpack)/buildin/global.js 509 bytes {1} [built]
[2] (webpack)/buildin/module.js 517 bytes {1} [built]
[3] ./src/index.js 216 bytes {0} [built]
[4] multi lodash 28 bytes {1} [built]
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.
🎉
@skipjack Added some comments, but other than that this is looking really good. When I'm tackling |
@TheDutchCoder awesome feedback, thank you! I will make these changes soon. |
@skipjack more than welcome! |
Ok applied the first round of changes and I'll give this another read through tomorrow night. @bebraw @jakearchibald you should both feel free to review as well. |
content/api/module-methods.md
Outdated
|
||
`webpackMode`: Since webpack 2.6.0, different modes for resolving dynamic imports can be specified. The following options are supported: | ||
|
||
- `"lazy"` (default): Generates a chunk per request, meaning everything is lazy loaded. |
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.
It isn't clear to me what a "request" is. Is a "request" a call to import(moduleSpecifier)
? Meaning "lazy"
creates a chunk for each moduleSpecifier
passed to import()
?
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.
Yes I believe for an expression like import(`./locales/${code}.json`)
, "lazy"
will generate a chunk for every possible import, so that once the code
variable is processed, each JSON file can be loaded individually. And "lazy-once"
would generate a single chunk containing all possible JSON files that would only be loaded on the first execution of import()
(or what it's transformed to) at runtime.
content/api/module-methods.md
Outdated
`webpackMode`: Since webpack 2.6.0, different modes for resolving dynamic imports can be specified. The following options are supported: | ||
|
||
- `"lazy"` (default): Generates a chunk per request, meaning everything is lazy loaded. | ||
- `"lazy-once"`: Generates a single chunk for all possible requests. The first call initiates a network request for all modules, and any following requests are already fulfilled. This is only available for when importing an expression. |
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.
The word "request" is used interchangeably to mean network request and call to import()
.
Potential alternative:
"lazy"
(default): Generates a lazy-loadable chunk for each import()
ed module.
"lazy-once"
: Generates a single lazy-loadable chunk that can satisfy all calls to import()
. The chunk will be fetched on the first call to import()
, and subsequent calls to import()
will use the same network response.
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.
This is only available for when importing an expression.
It isn't clear what this means.
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.
See my comment above, "expression" means something like this: import(`./locales/${code}.json`)
. Meaning a partially dynamic statement that gives webpack enough information to do its bundling but will still be evaluated at runtime to get the remaining information (e.g. code
).
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'm working on updating this section now to clarify these things.
content/api/module-methods.md
Outdated
|
||
`webpackMode`: Since webpack 2.6.0, different modes for resolving dynamic imports can be specified. The following options are supported: | ||
|
||
- `"lazy"` (default): Generates a chunk per request, meaning everything is lazy loaded. |
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'm not sure what "request" means here. Does it mean each call to import(moduleSpecifier)
?
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.
See my comment above.
content/api/module-methods.md
Outdated
|
||
- `"lazy"` (default): Generates a chunk per request, meaning everything is lazy loaded. | ||
- `"lazy-once"`: Generates a single chunk for all possible requests. The first call initiates a network request for all modules, and any following requests are already fulfilled. This is only available for when importing an expression. | ||
- `"eager"`: Generates no chunk. All modules are included in the current chunk and no additional network requests are made. A `Promise` is still returned but is already resolved. In contrast to a static import, the module isn't executed until the request is made. |
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.
Replace "request" with "call to import()
" or similar
content/api/module-methods.md
Outdated
|
||
W> Fully dynamic statements, such as `import(foo)`, __will fail__ because webpack requires at least some file location information. This is because `foo` could potentially be any path to any file in your system or project. The `import()` must contain at least some information about where the module is located, so bundling can be limited to a specific directory or set of files. | ||
|
||
W> The entire module namespace is included. For example, ``import(`./locale/${language}.json`)`` will cause every `.json` file in the `./locale` directory to be bundled into the new chunk. At run time, when the variable `language` has been computed, any file like `english.json` or `german.json` will be available for consumption. |
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.
The entire module namespace is included.
I'm not sure this makes sense in the context. What is "namespace" here? Does it mean "all exports"?
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.
By "namespace" I think the original author meant...
every module that could potentially be requested by a statement like
import(`./locales/${code}.json`)
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'll clarify.
content/guides/code-splitting.md
Outdated
}; | ||
``` | ||
|
||
With the `CommonsChunkPlugin` in place, we should now see the duplicate dependency removed from our `index.bundle.js`. The plugin should notice that we've separated `lodash` out to a separate chunk and remove the dead weight from our main bundle. Let's do an `npm run build` to see if it worked: |
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.
Will the reader know that npm run build
will run webpack via a script in packages.json
?
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.
Yes, it is explained in the first guide, but we should maybe link to it here (we can't assume everyone goes through all the guides sequentially, so cross-linking is a good idea)
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 think I forgot to add the...
This guide is a follow up to Getting Started and Output Management...
at the beginning. Maybe I should link to just one of those two -- I guess you were thinking this would be a good follow up to Output Management @TheDutchCoder?
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.
Yes I think that this would be a good follow up Output Management once I start working on it haha
content/guides/code-splitting.md
Outdated
Version: webpack 2.6.1 | ||
Time: 527ms | ||
Asset Size Chunks Chunk Names | ||
index.bundle.js 542 bytes 0 [emitted] index |
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.
A script coming out this small looks like an error to me. Maybe you need to make it clear that virtually nothing is happening in this script other than importing lodash.
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.
Actually, it imports lodash, creates a new element, sets its innerHTML, and then appends it to the DOM.
It's a tiny script indeed, and just for illustration, but I don't think it would throw people off.
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.
It's probably ok.
When I was trying to figure out CommonsChunkPlugin
I sometimes ended up with everything ending up in the "vendor" chunk, so I guess that's why I associate a small index bundle with "shit went bad" 😄
- | ||
- // Lodash, now imported by this script | ||
- element.innerHTML = _.join(['Hello', 'webpack'], ' '); | ||
+ return import(/* webpackChunkName: "lodash" */ 'lodash').then(module => { |
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.
The purpose of this comment isn't mentioned
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.
Added some comments about this and a link to the reference in the paragraph below.
content/guides/lazy-loading.md
Outdated
|
||
## Example | ||
|
||
Let's take the example from [Code Splitting](/guides/code-splitting#dynamic-imports) and tweak it a bit to demonstrate this concept even more. The code there does cause a separate chunk, `lodash.bundle.js`, to be generated and technically "lazy-loads" it as soon as the script is run. The trouble is that no user interaction is required to load the bundle -- meaning that every time the page is loaded, the request will fire. This doesn't help us too much, and actually may impact performance negatively. |
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.
may impact performance negatively.
will, surely?
+ var print = module.default; | ||
+ | ||
+ print(); | ||
+ }) |
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 realise the guide should be as simple as possible, but this should show a spinner since UI is blocked on network. Maybe just mention it as a note? May also be worth linking to <link rel="preload">
https://developers.google.com/web/updates/2016/03/link-rel-preload, so you can handle the download in the background and have it ready by the time the user clicks.
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.
That's a good idea, if we can keep it easy to understand.
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.
Added a comment above the click handler to indicate this. Because this guide is relatively short, I actually wouldn't be opposed to you updating this example to really display one. Maybe a follow up PR?
3e4360c
to
8097f98
Compare
Ok I just cleaned up this branch and added some final commits. Once the build passes I'll squash and merge. @TheDutchCoder once it's merged feel free to give these guides one last pass and pr any updates you think might be needed. I think it's cleaner that way as this PR is becoming harder to follow for me (with all the old comments and diffs) and what's here now is a major improvement over the existing guides. I'll start reviewing your other PRs once this sucker is merged. |
filename: '[name].bundle.js', | ||
path: path.resolve(__dirname, 'dist') | ||
}, | ||
plugins: [ |
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.
Minor: In other examples we put plugins
before output
. I don't care much about the order of things, but it might be nice to keep it consistent.
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 agree, I'll change in a follow up PR. I actually like the order of entry
=> module
=> plugins
=> (anything else) => output
as it kind of reflects the order of operations.
[3] ./src/another-module.js 87 bytes {1} [built] | ||
[4] ./src/index.js 216 bytes {0} [built] | ||
``` | ||
|
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.
Make mention of a possible index.html
as output as well?
It depends on how we tie this guide to the previous one, where a template is provided to the HtmlWebpackPlugin.
Let's wait with finalizing the outputs until Output Management
is merged, so we know for sure.
+ var print = module.default; | ||
+ | ||
+ print(); | ||
+ }) |
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.
<nitpick>
Add semi colon :)</nitpick>
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.
😆 will do -- I'm in the ASI boat myself but I'll fix for consistency.
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.
Same here, it's hard to switch back haha
💯 Some minor things we can add in a later PR, but this is looking fantastic! |
This seems to have broken links to https://webpack.js.org/guides/code-splitting-async/. Could a redirect be put in place? E.g. linked from https://eng.uber.com/m-uber/ and various other places. |
@Krinkle good catch, will do. I'll add some for the other removed removed pages as well while I'm at it. |
This pr attempts to:
import()
documentation to the Module API page.lazy-load-react
with a short, framework-agnosticlazy-loading
page.Resolves #1333 (though some of that discussion relates to #652)
cc @TheDutchCoder @jakearchibald -- I'll update this again once it's ready for review which should be within the next hour or two.
@TheDutchCoder I could use your help with a few sections in the examples that I left as TODOs for now. Before it's merged, or in a follow-up PR, we'll make sure they are fully in sync with those from
getting-started
.@jakearchibald I read through your suggested edits and the other comments from #1333. Seeing as I changed the direction of this guide a bit, I think most of your discussed changes would make more sense in the Caching guide (which would mention Code Splitting as a pre-requisite). If you'd be up for tackling that guide in a follow up PR, which should take care of #652, that would be amazing.