Update comments for loaders webpack to clarify excluding file extensions #1500
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@gaearon - Sorry about that, thanks for picking those bad merges out. I had literally just forked to create a new react-script, and decided to do one of the open tickets. Didn't even think about the fact that I had modified master. Updated with the request change, and all the other stuff removed. (If only you hadn't checked at all, we could have finally had CSS modules in CRA 🤘) |
|
Hi @ro-savage! Thanks for your contribution. I really like the addition of the Do you think we could shorten the explanation to be short and sweet? |
| @@ -179,6 +178,10 @@ module.exports = { | |||
| name: 'static/media/[name].[hash:8].[ext]' | |||
| } | |||
| } | |||
| // *STOP* Are you adding a new loader? | |||
| // Remember to update the exclude list in the "url" loader to exclude the new extension. | |||
There was a problem hiding this comment.
How about just:
** STOP ** Are you adding a new loader?
Remember to add the new extension(s) to the exclusion list.
See above for details.
| // *STOP* Are you adding a new loader? | ||
| // Remember to update the exclude list in the "url" loader to exclude the new extension. | ||
| // For example, when adding a loader to handle .sass remember to add /\.sass$/ | ||
| // to "url" loaders exclude list |
| // The "url" loader exclude list needs to be updated with every change of extensions | ||
| // that other loaders match. | ||
| // For example, when adding a loader for a new supported file extension (such as .sass), | ||
| // you need to add the supported extension (/\.sass$/) to this loader too. |
There was a problem hiding this comment.
I think this could be easier to consume, how about this?
** STOP **
This loader handles all assets unless explicitly excluded.
The `exclude` list *must* be updated with every change to loader extensions.
It should contain one line for each loader.
** ADDING NEW LOADERS **
When adding a new loader, you must add its `test` as a new entry in the `exclude` list for this loader.
If you do not add the entry, your new loader may not work if it introduced a new extension.
"file" loader makes sure those assets end up in the `build` folder.
-snip-
|
I'm not sure if just "Important" catches my eye. I liked the stars. And I believe this is important enough to yell at the user (all caps 😆). Also, I think the example with I like shortening of the "file"/"url" explanation. Maybe we could add a bit more tho: edit: as for the truthness, "url" falls back to "file". |
|
Ignore this. Github is being weird. It showed multiple comments. I deleted one. and then it deleted both. And I couldn't see the changes requested. I will update PR @Timer Updated the original comment with your suggestions. I like giving a real example. Examples are often clearer than trying to 'explain' how/why to do something. Happy to remove it though the whole ' |
|
Personally, I think "you must add its If you disagree, let me know. |
|
Updated per requested changes, with a few minor modifications. Yes I agree that saying |
| // "url" loader works like "file" loader except that it embeds assets | ||
| // smaller than specified limit in bytes as data URLs to avoid requests. | ||
| // A missing `test` is equivalent to a match. | ||
| // ** ADDING/UPDATING NEW LOADERS ** |
|
Looks good! Just one tiny change please. :) |
|
Doh, thats what happens when making PR changes while in the middle of a meeting. Thanks for the patience. Good to go. |
|
If not too much trouble could you rebase against master? If not I can do it. |
|
First time doing that. Hopefully done correctly. Should now be rebased with master. Let me know if I did something wrong. |
|
Perfect, thanks! |
|
This is perfect. Thanks. |
As per issue #1208 this PR updates the comments explaining that after ejecting and trying to add new loaders, the user must also add add the extension that loader is handling to the exclude list for the
url loader.I have made the original comment stand out more, and given a more practical example and have added a new comment to the edit of the
loadersarray where a person would usually add a new loader to warn them to update the exclude list.Happy to hear feedback if this is over the top / not obvious enough.