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

Shimming Rewrite #1579

Merged
merged 13 commits into from
Sep 12, 2017
Merged

Shimming Rewrite #1579

merged 13 commits into from
Sep 12, 2017

Conversation

skipjack
Copy link
Collaborator

@skipjack skipjack commented Sep 7, 2017

Rewrite the Shimming guide to synchronize with the others. This is the last core guide we identified in the list from #1258. Once this is done we can close that out, finish the guides structuring in #1519, and begin cleaning up the other sections as well.

Closes #1258

Briefly mention the other utilities (I have this content saved in
another file atm) and begin work on _Polyfills_ section.
fetch('https://jsonplaceholder.typicode.com/users')
.then(response => response.json())
.then(json => {
console.log('We retrieved some data! AND we're confident it will work on a variety of browser distributions.')
Copy link

Choose a reason for hiding this comment

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

we\'re

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks! Will fix that now as I'm wrapping up the remaining content. What'd you think of the rest of the updates?

Copy link

Choose a reason for hiding this comment

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

I'm not sure about the fetch/polyfills example: I probably wouldn't use it like that, because relying on global side effects when doing dynamic imports sounds wrong and just begs for race conditions. The previous example where they used script tags is more realistic.

Also, why did you remove noParse docs - are they mentioned elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just added noParse back to the bottom in Other Utilities. As for the fetch / polyfills example, yeah I think you may be right. I mentioned why I did it in my comment below but maybe I'll change that back. Thanks for your feedback! Feel free to do another review now that everything is pushed.

@skipjack
Copy link
Collaborator Author

skipjack commented Sep 9, 2017

@TheDutchCoder and @jeremenichelli ok I think this is pretty much good to be reviewed.

@jeremenichelli I changed the on demand polyfills examples slightly to use dynamic import() statements instead of a script in the index.html file as we recommend that people use the HTMLWebpackPlugin in Output Management and it's a bit harder to dynamically include scripts.

The only section I removed from the guide was this one:

Prefer Unminified Distributions

Most modules link the dist version in the main field of their package.json. While this is useful for most developers, for webpack it is better to alias the src version because this way webpack is able to optimize dependencies better. However in most cases dist works fine as well.

// webpack.config.js

module.exports = {
  ...
  resolve: {
    alias: {
      jquery: "jquery/src/jquery"
    }
  }
};

as I think that's more of an optimization than an example of shimming.

```

T> Note that we aren't binding the `import` to a variable. This is because polyfills simply run on there own, prior to the rest of the code base, allowing us to then assume certain native functionality exists.
Copy link

Choose a reason for hiding this comment

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

on their own


When there is no AMD/CommonJS version of the module and you want to include the `dist`, you can flag this module as [`noParse`](/configuration/module/#module-noparse). Then `webpack` will just include the module without parsing it, which can be used to improve the build time.

W> Any feature requiring the AST, like the `ProvidePlugin`, will not work.
Copy link

Choose a reason for hiding this comment

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

And I think require() is not parsed, so any dependencies will not be included in the bundle. This is probably worth mentioning.

```

T> Any script added dynamically like in the example above will run as soon as it's parsed, but we need our polyfill to run before our bundle. This is why we are setting `async` to `false` for each script.
Now, if we run our build a few more bundles will be emitted and everything should still run smoothly in the browser. Note that this set up could likely be improved upon but it should give you a good idea of how you can provide polyfills only to the users that need them.

Copy link
Member

Choose a reason for hiding this comment

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

I really like the reorganization on this section, kudos!

The only confusing part is this one. First thing, I would put the content of fetch.js before. When coming across index.js I was confused because I didn't know what fetch.js did.

Other thing, I don't know how applicable the example is to real cases. I know that the script on index approach is not as clean and diverge more form webpack documentation, but in general is the kind of loading strategy devs will need since usually the whole bundle relies on native APIs and not just small parts of it. Food for thought I guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, @asapach mentioned the same thing -- I think I'll switch it back to the original example you had.

@skipjack
Copy link
Collaborator Author

Ok made all the suggested changes. Should be ready for another review.

```

We can also use the `ProvidePlugin` to expose a single export of a module by configuring it with an "array path" (e.g. `[module, child, ...children?]`). So let's we only wanted to provide the `join` method from `lodash` wherever it's invoked:
Copy link

Choose a reason for hiding this comment

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

So let's we...

So let's imagine we ...

```

In your html file you need to conditionally load the `polyfills.js` file before your bundle. How you make this decision depends on the technologies and browsers you need to support.
With that in place, we can add the logic to conditionally load our new `polyfills.bundle.js` file. How you make this decision depends on the technologies and browsers you need to support. We'll just do some simple testing to determine whether our polyfills are needed:

Copy link
Member

Choose a reason for hiding this comment

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

The name of the new emmitted bundle would be polyfills.js, instead of polyfills.bundle.js

Copy link
Collaborator Author

@skipjack skipjack Sep 10, 2017

Choose a reason for hiding this comment

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

See the webpack config change: filename: '[name].bundle.js',.

It would be polyfills.bundle.js. I think it's better that way as it's clear that the src/polyfills.js went through webpack instead of being loaded directly. Let me know if I'm missing something here.

@skipjack
Copy link
Collaborator Author

Ok finally got the build to pass after some linting issues. @asapach @jeremenichelli thanks for the reviews and please let me know if there's anything else that could be clarified. I'll give it until tomorrow night (EST) to merge.

@asapach btw would you be interested in being a contributor? You caught a lot of things in the content here and it would be great if we could request your review on future PRs.

@asapach
Copy link

asapach commented Sep 11, 2017

@skipjack, thanks. I jumped on this PR because i have some practical experience with the subject matter. As much as I'd love to contribute more, I'm afraid I don't have a lot of time available. I'm watching the repo - if I see something I can help with I'll pop in.

@skipjack
Copy link
Collaborator Author

@asapach totally understand. Definitely feel free to chime in whenever.

@jeremenichelli
Copy link
Member

All good Greg, great work on this rewrite 👏

@skipjack skipjack merged commit e8dc2f3 into master Sep 12, 2017
@skipjack skipjack deleted the shimming-rewrite branch September 12, 2017 00:55
@skipjack
Copy link
Collaborator Author

@jeremenichelli @asapach merged, thanks again for the reviews.

@TheDutchCoder feel free to add more comments on here if you see anything and we can resolve in another PR.

+ </script>
</head>
<body>
<script src="bundle.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

I know that this was already merged, but I just spotted this on the site though I could be wrong... now that the output is [name].bundle.js will index bundle be index.bundle.js instead of just index.js?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good catch I missed that.


## Prefer unminified CommonJS/AMD files over bundled `dist` versions.
Another instance where _shimming_ can be useful is when you want to [polyfill](https://en.wikipedia.org/wiki/Polyfill) browser functionality to support more users. In this case, you may only want to deliver those polyfills to the browsers that need patch (i.e. load them on demand).
Copy link
Collaborator

Choose a reason for hiding this comment

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

g/that need patch/that need patching

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@skipjack
Copy link
Collaborator Author

@jeremenichelli @TheDutchCoder either of you have time to PR these minor fixes? I'm quite busy today... if not I'll tackle as soon as I have time.

@jeremenichelli
Copy link
Member

jeremenichelli commented Sep 12, 2017 via email

aladdin-add referenced this pull request in docschina/webpack.js.org Oct 10, 2017
* docs(api): minor fix in cli

Make minor fix in the CLI documentation to reflect this comment:

webpack#1577 (comment)

* docs(plugins): clarify and simplify `SourceMapDevToolPlugin` docs (#1581)

Try to resolve the confusion from #1556. Also update the options
section to clean things up a bit both for readability and grammar.

Resolves #1556

* docs(guides): update "external limitations" example in author-libraries (#1583)

* docs(plugins): update commons-chunk-plugin.md (#1560)

Changed example to declare the plugin within `plugins` property.

* docs(guides): rewrite shimming (#1579)

Rewrite the main sections of the guide to synchronize it with
the other core guides. Briefly mention the other utilities that
don't require full on examples (details can be found via links).

Closes #1258

* docs(guides): fix small issues in shimming (#1591)

* docs(guides): fix entry path in typescript (#1592)

* docs(guides): fix typos in production (#1584)

* fix(sponsors): update to reflect opencollective change (#1588)

OpenCollective's structure changed a bit. This address that
and makes a few other notable changes:

- add additional sponsors
- allow to merge additional sponsors to existing sponsors
- limit width of logos to 3 x height to disallow very wide logos
- format money values with commas

* docs(config): update configuration-languages (#1590)

Remove local type def from TypeScript snippet. The @types/node type 
definition module already declares `__dirname` type:

https://git.io/v5Dr9

* docs(guides): update hot-module-replacement (#1539)

Add an example to demonstrate using hot module replacement with 
`webpack-dev-server`'s Node.js API instead of within a normal
configuration file.

* docs(guides): update development.md (#1586)

The article was missing references to `clean-webpack-plugin`
in the `webpack.config.js` file and `package.json` files.

* docs(guides): update tree-shaking.md (#1589)

As a conclusion, I thought it would be a good idea to show case 
a high-level explanation of why it's called tree shaking. Once the 
user reads the guide and ends up with a high-level explanation 
the word tree shaking will stick to the user's head.

* docs(guides): update code-splitting (#1585)

Suggest solutions to the problem right away seems to be a 
better approach. Use similar text as was used here:

https://webpack.js.org/api/module-methods/#import-

* docs(plugins): update module-concatenation-plugin (#1565)

Add more information about optimization bailouts. Attempt to transfer 
content from the blog post to the official docs. I want to follow up with 
a PR for better bailout reasons. The hope is that the reasons will match 
those listed in the table.

* docs(api): fix type in compiler.md (#1601)

* docs(config): update output.md (#1541)

Clarify interactions between `libraryTarget` and `library`. The 
interactions between the two and how some of the sections were not
well explained, in some cases were incorrect, misleading or missing
information.

* docs(api): add concurrent usage warning in node.md (#1599)

* docs(guides): update environment-variables (#1549)

Add documentation about setting environment variables in CLI with 
`--env`. Link to webpack _CLI Environment_ section. Add additional 
usage to `--env` CLI docs.

* Swap ordering of pre/post loaders (#1602)

`pre` loaders run at the beginning, `post` loaders run at the end.

* docs(sponsors): update segment's donations (#1603)

* update contributors
dear-lizhihua referenced this pull request in docschina/webpack.js.org Oct 17, 2017
* update /content/loaders & /content/plugins

* docs(api): minor fix in cli

Make minor fix in the CLI documentation to reflect this comment:

webpack#1577 (comment)

* docs(plugins): clarify and simplify `SourceMapDevToolPlugin` docs (#1581)

Try to resolve the confusion from #1556. Also update the options
section to clean things up a bit both for readability and grammar.

Resolves #1556

* docs(guides): update "external limitations" example in author-libraries (#1583)

* docs(plugins): update commons-chunk-plugin.md (#1560)

Changed example to declare the plugin within `plugins` property.

* docs(guides): rewrite shimming (#1579)

Rewrite the main sections of the guide to synchronize it with
the other core guides. Briefly mention the other utilities that
don't require full on examples (details can be found via links).

Closes #1258

* docs(guides): fix small issues in shimming (#1591)

* docs(guides): fix entry path in typescript (#1592)

* docs(guides): fix typos in production (#1584)

* fix(sponsors): update to reflect opencollective change (#1588)

OpenCollective's structure changed a bit. This address that
and makes a few other notable changes:

- add additional sponsors
- allow to merge additional sponsors to existing sponsors
- limit width of logos to 3 x height to disallow very wide logos
- format money values with commas

* docs(config): update configuration-languages (#1590)

Remove local type def from TypeScript snippet. The @types/node type 
definition module already declares `__dirname` type:

https://git.io/v5Dr9

* docs(guides): update hot-module-replacement (#1539)

Add an example to demonstrate using hot module replacement with 
`webpack-dev-server`'s Node.js API instead of within a normal
configuration file.

* docs(guides): update development.md (#1586)

The article was missing references to `clean-webpack-plugin`
in the `webpack.config.js` file and `package.json` files.

* docs(guides): update tree-shaking.md (#1589)

As a conclusion, I thought it would be a good idea to show case 
a high-level explanation of why it's called tree shaking. Once the 
user reads the guide and ends up with a high-level explanation 
the word tree shaking will stick to the user's head.

* docs(guides): update code-splitting (#1585)

Suggest solutions to the problem right away seems to be a 
better approach. Use similar text as was used here:

https://webpack.js.org/api/module-methods/#import-

* docs(plugins): update module-concatenation-plugin (#1565)

Add more information about optimization bailouts. Attempt to transfer 
content from the blog post to the official docs. I want to follow up with 
a PR for better bailout reasons. The hope is that the reasons will match 
those listed in the table.

* docs(api): fix type in compiler.md (#1601)

* docs(config): update output.md (#1541)

Clarify interactions between `libraryTarget` and `library`. The 
interactions between the two and how some of the sections were not
well explained, in some cases were incorrect, misleading or missing
information.

* docs(api): add concurrent usage warning in node.md (#1599)

* docs(guides): update environment-variables (#1549)

Add documentation about setting environment variables in CLI with 
`--env`. Link to webpack _CLI Environment_ section. Add additional 
usage to `--env` CLI docs.

* update /content/loaders & /content/plugins

* Swap ordering of pre/post loaders (#1602)

`pre` loaders run at the beginning, `post` loaders run at the end.

* docs(sponsors): update segment's donations (#1603)

* fix(fetch): fix some issues when parsing remote content (#1604)

The previous regex was a simpler approach to the lead in content
issue in fetched plugin/loader readmes, however it wasn't targeted
enough to correctly remove all lead in content. The new approach
should be a bit more stable but honestly the real fix here would be
so better standardize/enforce the readme template before including
these packages in the documentation site. I also think we can simplify
the template a bit to make the parsing of these readme easier.

Resolves #1600

* docs(guides): fix typo in code-splitting (#1606)

* docs(config): update `inline` recommendation in dev-server (#1540)

Update tip based on discussion in #1540.

* docs(config): update pfx description in dev-server (#1595)

Clarify the difference between using `pfx` via the command line and
in a configuration.

* docs(config): clarify stats option sort and default (#1596)

* docs(config): add before/after hook info to dev-server (#1608)

* docs(guides): fix typo in development (#1609)

* docs(guides): fix typo in production.md (#1610)

* docs(readme): fix typos (#1615)

* fix(markdown): fix issue with multiple pipes in a single cell (#1611)

Introduced better fix for pipes escaping in Markdown table cells.

* docs(guides): update hot-module-replacement (#1614)

Second argument of `server.listen` is a hostname, in this context, 
`'localhost'`. The third argument of `server.listen` is the success 
callback...

https://github.com/webpack/webpack-dev-server/blob/master/lib/Server.js#L552

* docs(guides): fix `onClick` instances in caching (#1613)

* docs(config): fix typo in module.md (#1618)

* docs(guides): fix grammar in getting-started.md (#1619)

* docs(config): add filename to index example (#1620)

* docs(guides): update wds options in hot-module-replacement (#1617)

Define the host property of the options object and set it to 'localhost'. The 
webpack-dev-server CLI defaults the host to 'localhost', but `addDevServerEntrypoints` 
from the webpack-dev-server Node.js API does not set a default and leaves 
it undefined, which will eventually throw "Uncaught SyntaxError: The URL 
'http:/sockjs-node' is invalid" in the browser when following the code example.

webpack/webpack-dev-server#1129

* fix(sponsors): opencollective api changed, add validation (#1622)

* docs(development): update how-to-write-a-loader.md (#1621)

Changes `_` to `.` to have correct code.

* update /content/loaders & /content/plugins

* update contributors

* docs(plugins): add deepChildren to commons-chunk-plugin.md (#1625)

* feat(sponsors): highlight latest sponsors/backers (#1628)

* refactor: update information architecture

This commit contains the following changes:

- Merge support and development into a coherent "Contribute" section.
- Flatten `/api/plugins` in preparation for flattened page `group`ing.
- Sort "Contribute" section and rewrite index page using "Why Support?".
- Simplify and organize antwar config as much as possible.
- Move `title` / `description` fields to the actual components.

BREAKING CHANGE: The route restructuring and content reorg
breaks some routes. Before this is merged, we should make sure
to add redirects for anything broken.

* refactor(navigation): update routes and styling

Simplify routing to reflect antwar and content changes and remove
extraneous links. Simplify styling to first improve the design, but also
to provide a lot more breathing room so less hacks are needed for
smaller page sizes. With the updated design we should have room
for "Voting" and "Analyze" links once those micro apps are in a
better place.

* fix: add link to starter kits

"Support" is now included in the "Contribute" section and already has
a link in the top navigation. The starter-kits page was previously hidden
from users, so this exposes it a bit more.

* refactor(navigation): update the navigation component

Make minor styling change for consistent gutter. Extract link data into
a separate JSON file. Update variable and parameter names for clarity.

* feat: allow page grouping within sections via `group` field

Add support for a new `group` field in each page's YAML frontmatter to
group pages together in an intuitive way. The benefit of this approach vs
directories is that changing a pages group within the same top-level section
won't affect its route and thus will not break links to it.

* style(page): align content better on larger screens

* docs: remove empty pages

* docs(contribute): add writers-guide and update release-process

* docs: sort the base pages more intuitively

* docs(api): update page names and grouping

* fix(mobile): clean up mobile sidebar data

* docs(api): clean up formatting and fix linting errors

* docs(api): fix markdown listing issues

* docs(contribute): fix markdown linting issues

* docs(api): rewrite the index page

* docs(api): minor formatting and cleanup

* docs: add redirects for all broken routes

The changes in #1612 break a significant amount of pre-existing URLs, this
commit adds redirects for everything I could see that would be broken.

* style(page): fix small padding issue

The padding here is useful for browsers that display a full on scrollbar
instead of an overlay scrollbar. We can continue to think about how
that space it utilized in future commits/prs.

* docs(guides): update production.md (#1629)

Modify the doc accordingly to the following tweetstorm where Dan 
Abramov & Sean Larkin kindly explained this to me.

https://twitter.com/TheLarkInn/status/917524723579338752

* fix(markdown): wrap inline code (#1634)

This change makes inline code blocks wrap so long lines will not 
be hidden outside paragraph.

Resolves #1633

* docs(guides): fix small typo excluding closing brace (#1637)

* docs(guides): fix typo in development.md  (#1638)

* docs(guides): update hot-module-replacement.md (#1639)

Maintain consistent code from previous guide (development).

* docs(guides): maintain consistent code in production.md (#1640)

* docs(guides): update typescript.md

Configure TypeScript to compile modules to ES2015 module syntax,
enabling webpack to resolve the `import` / `export` syntax on it's own.
With this included, the only thing necessary to enable _tree shaking_
is the inclusion of the `UglifyJSPlugin` (which I'm not adding to this
article as it is just centered on getting started with TypeScript).

Resolves #1627

* update /content/loaders & /content/plugins

* fix LinkDropdown

* 修复 npm 命令错误导致编译不成功的问题
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.

Guides - Review and Simplify
4 participants