Add polyfills on demand section to shimming page #1528
Conversation
|
Beautiful, thanks for starting this @jeremenichelli! I will review in detail this weekend. @TheDutchCoder feel free to review as well. I think we can review, get this merged, then do a follow up pass to synchronize and clean it up. One step closer to knocking down #1258. |
There was a problem hiding this comment.
@jeremenichelli this looks great. I caught a few minor things but I think we should be able to get this merged soon. Please let me know if you have any questions.
Note that we will have to rework this content a bit when we synchronize it to complete #1258. I think @TheDutchCoder or I can pick that up though as we're already pretty familiar with the rest of the guides. May ping you for review/questions though.
content/guides/shimming.md
Outdated
|
|
||
| ## Loading polyfills on demand | ||
|
|
||
| It's really common in web projects to include polyfills in the main bundle. This is not recommended since we are penalizing modern browsers users, since they will have to download a bigger file unnecessarely. |
There was a problem hiding this comment.
"since" twice in the last sentence is a bit repetitive and "unnecessarely" is spelled wrong. Maybe:
It's really common in web projects to include polyfills in the main bundle. This penalizes modern browser users as they will be forced to download a bigger file containing unneeded polyfills.
There was a problem hiding this comment.
@jeremenichelli did you see this comment? I see the other changes but this sentence looks the same and "unnecessarely" is still spelled wrong (except that now it's highlighted 😆 ). Aside from this I think this PR is pretty much good to go, though we will have to rebase to resolve conflicts.
There was a problem hiding this comment.
lol my bad, fixing...
There was a problem hiding this comment.
@jeremenichelli no problem, if you could rebase on the latest master branch that would be appreciated as well. If not, I can handle it once you've made this last correction.
content/guides/shimming.md
Outdated
|
|
||
| ### References | ||
| * [Reward modern browser users script](https://hackernoon.com/10-things-i-learned-making-the-fastest-site-in-the-world-18a0e1cdf4a7#c665) | ||
| * [`useBuiltIns` in `babel-preset-env`](https://github.com/babel/babel-preset-env#usebuiltins) |
There was a problem hiding this comment.
Instead of creating this whole section in markdown, use the YAML frontmatter shortcut:
title: ...
sort: ...
contributors:
- ...
related:
- title: Reward modern browser users script
url: https://hackernoon.com/10-things-i-learned-making-the-fastest-site-in-the-world-18a0e1cdf4a7#c665
- title: ...
url: ...
This will cause them to pop up in a consistent Further Reading section at the bottom of the page.
content/guides/shimming.md
Outdated
|
|
||
| ### Smaller babel polyfill | ||
|
|
||
| `babel-preset-env` uses [browserslist](https://github.com/ai/browserlist) to transpile only what is not supported in your browsers matrix. This preset comes with the `useBuiltIns` option _(false by default)_ which converts your global `babel-polyfill` import to a more granular feature by feature import pattern like: |
There was a problem hiding this comment.
The CI build is failing currently because this link is a 404:
https://github.com/ai/browserlist
Was that repo moved?
|
@skipjack I just pushed a fix realted to your first review past. Regarding the general section clean and rewrite issue, I want to help reviewing not only the section I just wrote but all the page if you think it will help the team, just make sure to tag me as reviewer <3 |
content/guides/shimming.md
Outdated
|
|
||
| ## Loading polyfills on demand | ||
|
|
||
| It's really common in web projects to include polyfills in the main bundle. This is not recommended because we are penalizing modern browsers users since they will download a bigger file **unnecessarely**. |
There was a problem hiding this comment.
@jeremenichelli Remove 'really', in this sentence it doesn't add anything.
There was a problem hiding this comment.
Hi @jeremenichelli I'm spending some time with the webpack docs and polishing the language we use, so here are few small comments and suggestions.
Thanks for your time :)
|
@ChrisChinchilla thanks for the feedback :) just pushed changes regarding your suggestion. |
jeremenichelli
force-pushed
the
guides/polyfills
branch
2 times, most recently
from
98ee977 to
65c8112
Compare
|
@skipjack I fixed the initial paragraph ending, go checkout it out and tell me if you like it. Also, I tried to rebase but I got a weird |
@jeremenichelli looks great thanks. Yeah those |
|
Awesome @skipjack, feel free to tag me on the PR for Guides section cleaning, more than willing review/help. |
Add a short guide about how to load polyfills only when needed to not penalize users on modern browsers.
Resolves #938