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

[bebbi]

Loads all files in appSrc not already handled by other loaders.
Also, switch image loading from file loader to url-loader.

Fixes #667.

[bebbi]

Travis failing is because of line 78,

test -e build/static/media/*.svg

That is because the logo.svg is now packed into the bundle.
Updated e2e.sh, removed line.

[fson]

This looks very good, nice work @bebbi! I had a couple of suggestions how to simplify things even further.

[fson]

I think we'll be able to get rid of this separate loader for web fonts, since these file extensions are handled by the default url-loader above.

The only difference left seems to be that this config loads files outside the src folder. I'd say let's simplify the rules for handling files further:

• JavaScript is compiled with Babel only if it's in the src folder.
• All the other file types can be in any folder.

Then we can simply drop the special handling for web fonts, and the ways different file types are handled is easy to predict and understand.

[sapegin]

I see a reason to handle webfonts separately: If you have a font in four formats and all of them are less then 10K you’ll have four inlines files which is probably not what you expected. But that means we need file-loader here.

[fson]

I can see a similar argument could be made for other file types too. If anything, that could mean we need to make the size threshold smaller. Because handling them separately would mean going back to maintaining a whitelist, I would not make an exception for those file types at least without more data about how real bundle sizes are affected.

[fson]

Perhaps we could make the regex more readable by making use of the excludeoption and simply listing the patterns handled by other loaders verbatim?

exclude: [
  /\.(js|jsx)$/,
  /\.css$/,
  /\.json$/
]

Then we wouldn't have to do a negative lookahead and the list would also be very easy to keep up to date if new loaders get added.

[bebbi]

Agree with adding to exclude. But don't we need to add the querystring bit (\?.*)? with some/many of them? I don't have experience with this. If so, I suggest a single line

/\.(js|jsx|etc)(\?.*)?$/

which should be just as easy to edit. Agree?

[fson]

The idea was to include exactly the same regexes used in those other loaders, so each file is either handled by one of them or handled by this loader. The JS, JSON and CSS loader configs we have don't match query strings in their regexes, so there's no need to match them here either. I think the whole loader config could be as simple as:

{
  test: /./,
  loader: 'url',
  query: {
    limit: 10000,
    name: 'static/media/[name].[hash:8].[ext]'
  },
  exclude: [
    /\.(js|jsx)$/,
    /\.css$/,
    /\.json$/
  ]
}

[fson]

exclude: /\.(js|jsx|css|json)$/

would work the same way, but then it's no longer the same regex, so it's harder to notice if we introduce discrepancies between them.

[bebbi]

makes sense - it escaped me that the other loaders all don't have the querystring. will update.

[fson]

Also it seems that a loader without the test regex will match all files, so we can also omit that.

[bebbi]

done (added a note about missing test for clarity). Squashed to keep history clean.

[fson]

There's two similar checks further down this file that still fail the test in Travis. Feel free to remove them too. Also the existing # Check for expected output comment is probably enough since nobody will remember logo.svg used to be not inlined. 😉

[fson]

Looks good to me!

[gaearon]

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

[bebbi]

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

[gaearon]

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.

[bebbi]

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?

[bebbi]

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.

[gaearon]

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

[bebbi]

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.

[gaearon]

What about this line? It will produce different behaviors now.

Please search for all whitelists. For example by searching for jpg.

[bebbi]

looks like it's these

create-react-app/packages/eslint-config-react-app/index.js:
   45      'import/ignore': [
   46        'node_modules',
   47:       '\\.(json|css|ico|jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$',
   48      ],
   49      'import/extensions': ['.js'],

create-react-app/packages/react-scripts/utils/createJestConfig.js:
   22      moduleFileExtensions: ['jsx', 'js', 'json'],
   23      moduleNameMapper: {
   24:       '^.+\\.(ico|jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$': resolve('config/jest/FileStub.js'),
   25        '^.+\\.css$': resolve('config/jest/CSSStub.js')
   26      },

[gaearon]

Yep, looks so!

[gaearon]

Let's fix all of these in a single followup PR? Thanks!

[bebbi]

I'll look into all tomorrow AM UTC.

[gaearon]

Thanks, I appreciate it! @fson Let's block the release on this at least.