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

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

Merged
merged 1 commit into from
Nov 20, 2016

Conversation

bebbi
Copy link
Contributor

@bebbi bebbi commented Nov 18, 2016

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

Fixes #667.

@bebbi
Copy link
Contributor Author

bebbi commented Nov 18, 2016

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.

Copy link
Contributor

@fson fson left a comment

Choose a reason for hiding this comment

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

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

{
test: /\.(mp4|webm|wav|mp3|m4a|aac|oga)(\?.*)?$/,
test: /\.(eot|otf|svg|ttf|woff|woff2)(\?.*)?$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// "url" loader works like "file" loader except that it embeds assets
// smaller than specified limit in bytes as data URLs to avoid requests.
{
test: /\.(?!(js|jsx|css|json|html)(\?.*)?$)[^\.]+$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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$/
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -71,11 +71,10 @@ npm install

# Test local build command
npm run build
# Check for expected output
# Check for expected output (logo.svg is now inlined)
Copy link
Contributor

Choose a reason for hiding this comment

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

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. 😉

Loads all files not already handled by other loaders.
Also, switch image loading from file loader to url-loader.
@fson
Copy link
Contributor

fson commented Nov 20, 2016

Looks good to me!

@fson fson merged commit 1c622ec into facebook:master Nov 20, 2016
// 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.
Copy link
Contributor

@gaearon gaearon Nov 20, 2016

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.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

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

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

@bebbi
Copy link
Contributor Author

bebbi commented Nov 20, 2016

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
Copy link
Contributor

gaearon commented Nov 20, 2016

Yep, looks so!

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

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

@bebbi
Copy link
Contributor Author

bebbi commented Nov 20, 2016

I'll look into all tomorrow AM UTC.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2016

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

@fson fson added this to the 0.8.0 milestone Nov 23, 2016
jarlef pushed a commit to jarlef/create-react-app that referenced this pull request Nov 28, 2016
Loads all files not already handled by other loaders.
Also, switch image loading from file loader to url-loader.
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
Loads all files not already handled by other loaders.
Also, switch image loading from file loader to url-loader.
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
Loads all files not already handled by other loaders.
Also, switch image loading from file loader to url-loader.
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants