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.

Sign up for GitHub

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

Jump to bottom

Use url-loader with limit 10k as a default loader. #1059

Merged
merged 1 commit into from
Nov 20, 2016
Merged

Use url-loader with limit 10k as a default loader. #1059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions packages/react-scripts/config/webpack.config.dev.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,32 @@ module.exports = {
}
],
loaders: [
// Default loader: load all assets that are not handled
// by other loaders with the url loader.
// Note: This list needs to be updated with every change of extensions
// the other loaders match.
// E.g., when adding a loader for a new supported file extension,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a rather big gotcha. Can we place it last in the file or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put this loader first for exactly that reason :-). Maybe add another comment to all other loaders?

Copy link
Contributor

Choose a reason for hiding this comment

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

The gotcha would still apply right? Then I guess it doesn't make a difference.
I wish we could find a way to "validate" the config automatically though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose the "negative" loader could be calculated from the others, at the price of making it imperative.
The least-invasive change might remove the loader and add a line at the end of the config something like:

module.exports.loaders.push(fn(module.exports.loaders));

Would you like something along these lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or perhaps it would make more sense to run a validation function at the end of the config and throw a descriptive error if negative tests not matching the other loaders? Con: only naive string comparison for regexes, but I think it shouldn't be a problem.

// we need to add the supported extension to this loader too.
// Add one new line in `exclude` for each loader.
//
// "file" loader makes sure those assets get served by WebpackDevServer.
// When you `import` an asset, you get its (virtual) filename.
// In production, they would get copied to the `build` folder.
// "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.
{
exclude: [
/\.(js|jsx)$/,
/\.css$/,
/\.json$/
],
loader: 'url',
query: {
limit: 10000,
name: 'static/media/[name].[hash:8].[ext]'
}
},
// Process JS with Babel.
{
test: /\.(js|jsx)$/,
Expand Down Expand Up @@ -138,26 +164,6 @@ module.exports = {
{
test: /\.json$/,
loader: 'json'
},
// "file" loader makes sure those assets get served by WebpackDevServer.
// When you `import` an asset, you get its (virtual) filename.
// In production, they would get copied to the `build` folder.
{
test: /\.(ico|jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2)(\?.*)?$/,
loader: 'file',
query: {
name: 'static/media/[name].[hash:8].[ext]'
}
},
// "url" loader works just like "file" loader but it also embeds
// assets smaller than specified size as data URLs to avoid requests.
{
test: /\.(mp4|webm|wav|mp3|m4a|aac|oga)(\?.*)?$/,
loader: 'url',
query: {
limit: 10000,
name: 'static/media/[name].[hash:8].[ext]'
}
}
]
},
Expand Down
45 changes: 26 additions & 19 deletions packages/react-scripts/config/webpack.config.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,32 @@ module.exports = {
}
],
loaders: [
// Default loader: load all assets that are not handled
// by other loaders with the url loader.
// Note: This list needs to be updated with every change of extensions
// the other loaders match.
// E.g., when adding a loader for a new supported file extension,
// we need to add the supported extension to this loader too.
// Add one new line in `exclude` for each loader.
//
// "file" loader makes sure those assets get served by WebpackDevServer.
Copy link
Contributor

@gaearon gaearon Nov 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Comment about WDS shouldn't end up in the production config.
Notice how it wasn't before this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies. I re-arranged a few things and tried to mirror exactly - this escaped.

Let me know if you want me to follow up with another PR on this and whatever decision for above.

// When you `import` an asset, you get its (virtual) filename.
// In production, they would get copied to the `build` folder.
// "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.
{
exclude: [
/\.(js|jsx)$/,
/\.css$/,
/\.json$/
],
loader: 'url',
query: {
limit: 10000,
name: 'static/media/[name].[hash:8].[ext]'
}
},
// Process JS with Babel.
{
test: /\.(js|jsx)$/,
Expand Down Expand Up @@ -150,25 +176,6 @@ module.exports = {
{
test: /\.json$/,
loader: 'json'
},
// "file" loader makes sure those assets end up in the `build` folder.
// When you `import` an asset, you get its filename.
{
test: /\.(ico|jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2)(\?.*)?$/,
loader: 'file',
query: {
name: 'static/media/[name].[hash:8].[ext]'
}
},
// "url" loader works just like "file" loader but it also embeds
// assets smaller than specified size as data URLs to avoid requests.
{
test: /\.(mp4|webm|wav|mp3|m4a|aac|oga)(\?.*)?$/,
loader: 'url',
query: {
limit: 10000,
name: 'static/media/[name].[hash:8].[ext]'
}
}
]
},
Expand Down
3 changes: 0 additions & 3 deletions tasks/e2e.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,6 @@ npm run build
test -e build/*.html
test -e build/static/js/*.js
test -e build/static/css/*.css
test -e build/static/media/*.svg
test -e build/favicon.ico

# Run tests with CI flag
Expand Down Expand Up @@ -133,7 +132,6 @@ npm run build
test -e build/*.html
test -e build/static/js/*.js
test -e build/static/css/*.css
test -e build/static/media/*.svg
test -e build/favicon.ico

# Run tests with CI flag
Expand Down Expand Up @@ -163,7 +161,6 @@ npm run build
test -e build/*.html
test -e build/static/js/*.js
test -e build/static/css/*.css
test -e build/static/media/*.svg
test -e build/favicon.ico

# Run tests, overring the watch option to disable it.
Expand Down