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

Disable inline images for images that are less than 10,000 bytes #3437

Closed
nicgirault opened this issue Nov 10, 2017 · 14 comments · Fixed by #6060
Closed

Disable inline images for images that are less than 10,000 bytes #3437

nicgirault opened this issue Nov 10, 2017 · 14 comments · Fixed by #6060

Comments

@nicgirault
Copy link

Thanks for this awesome project.

I recently get a warning during build telling me:

The bundle size is significantly larger than recommended.
Consider reducing it with code splitting: https://goo.gl/9VhYWB
You can also analyze the project dependencies: https://goo.gl/LeUzfb

Analyzing the project dependencies, I discovered that the easiest and most significant improvement would be to get rid of these small images under 10,000 bytes that are imported as Data URI by webpack (it's 25% of the size of my app).

I don't see an option to disable the feature. Does it exists? If no I have currently 2 options that are:

  • moving the images in the public folder
  • ejecting the app

Both options are a big loss so an option to change the current behaviour would be nice.

Thanks for your help

@nicgirault
Copy link
Author

I propose to do a pull request to add an environment variable that would be used here: https://github.com/facebookincubator/create-react-app/blob/36cd35d684f6f0eda971b30c789010758fc61ceb/packages/react-scripts/config/webpack.config.prod.js#L166 and here https://github.com/facebookincubator/create-react-app/blob/70e0c08ef7de8845b1a4c6700822e27e45b481a8/packages/react-scripts/config/webpack.config.dev.js#L159 and would fallback to 10000.

@Timer
Copy link
Contributor

Timer commented Nov 10, 2017

Intriguing use case.
Even if this shaves off 25%, that means your bundle is still huge. If you split the application, the inlined images should also be split and distributed across the bundles.

Let's see what others have to say, but the third option is what I just mentioned.

edit: this option currently cannot be modified or disabled

@edmorley
Copy link

edmorley commented Mar 15, 2018

Another point to consider when using url-loader's inlining: small changes in either CRA's chosen threshold, or asset sizes in an app can mean a previously working Content-Security-Policy ends up blocking newly-inlined assets due to lack of data: in the img-src CSP policy directive.

For sites that have correct long-lived cache control headers (especially those remembering to use the immutable directive), and aren't as concerned about the initial cold cache case, IMO never inlining is preferable.

@peterbe
Copy link

peterbe commented Dec 18, 2018

Here's another attempt to solve this: #6060

It's similar to the idea of INLINE_RUNTIME_CHUNK. Now, running IMAGE_INLINE_SIZE_LIMIT=0 yarn run build will mean that every image in (src/*.(bmp|gif|jp?eg|png)) will be considered too big and not base64 into the JS.

@mzvast
Copy link

mzvast commented Jan 10, 2019

Any progress on this proposal?

@peterbe
Copy link

peterbe commented Jan 10, 2019

My PR is conflicting now but I'm not sure how to ping/pester it for review.

peterbe added a commit to peterbe/create-react-app that referenced this issue Jan 10, 2019
@andyfurniss4
Copy link

Adding my name to this because I don't want my images inlined!

@Rogdham
Copy link

Rogdham commented Mar 15, 2019

Websites dealing with CSP will likely add data: in the CSP header because of the inline images (something like img-src 'self' data:;). MDN states that this is not recommanded and can be insecure (emphasis is mine):

data: Allows data: URIs to be used as a content source. This is insecure; an attacker can also inject arbitrary data: URIs. Use this sparingly and definitely not for scripts.


The alternative for CSP is using a nonce-based approach, but it has the following drawbacks:

  • way more complex to put in place, as it requires server-side modification of some static files (plus the documentation is very scarce);
  • the nonce passed to __webpack_nonce__ is not applied to img tags (this may be a bug);
  • browsers without CSP version 2 support will fallback to CSP version 1, so the data: will need to be present in the CSP header anyways for CSP version 1;
  • nonce-based approach are not well-known yet, which means less documentation, more error-prone, and users are likely to use the other, simplest, solution.

All in all, it would be way simpler to just have an option to disable inline images. #6060 gets my +1 👍

@peterbe
Copy link

peterbe commented Mar 15, 2019

Messing with CSP just to bend-over-backward for this "flaw" in CRA isn't the right direction. I still think my pr should get reviewed and landed. And perhaps it should be changed to make the default minimum 0 bytes, meaning you have to deliberately set an env var if you want base64 inlining.

Hey! If you're here and also don't want base64 inlining you can...

  1. For images in the src/*.js don't do this:
import myImage from './foo.png';

<img src={myImage}>

Instead, do this:

const myImage = process.env.PUBLIC_URL + '/foo.png';

<img src={myImage}>
  1. In your src/App.js don't do this:
div.page { background: url(./bullet.png) no-repeat left center;

do this instead:

div.page { background: url(/static/bullet.png) no-repeat left center;

You'll also need to git mv src/foo.png public/static/images/ or whatever is equivalent in your project.

@some1else
Copy link

some1else commented Apr 1, 2019

But I want the images loaded, checksummed, optimized and whatnot. I just don't want them inlined. In my use case long strings are problematic (using React to render multipart emails).

@some1else
Copy link

Hopefully #6060 gets the stamp of approval

@Kizmar
Copy link

Kizmar commented Apr 10, 2019

Pile on reason: I realized a .png was being added as "data:" instead of using a path. At first I thought it was the file type because the same image as a .jpg worked. I don't want to have to add "data:" to my CSP as it's not recommended for security reasons. I'd like to be able to basically disable this feature all together and always use the image path.

@imyoka
Copy link

imyoka commented May 16, 2019

Adding my name to this because I don't want my images inlined!

@fredrivett
Copy link

We're in the exact same position as @Kizmar, images are being converted to data: with base64, which our CSP is blocking. It's not advised to allow data: URIs as content source, as MDN says:

data: Allows data: URIs to be used as a content source. This is insecure; an attacker can also inject arbitrary data: URIs. Use this sparingly and definitely not for scripts.

Ideally there would be a solution that enables us to keep our sites secure whilst still utilising the standard image processing (just without the base64 step).

@lock lock bot locked and limited conversation to collaborators Jun 23, 2019
MOZGIII pushed a commit to MOZGIII/create-react-app that referenced this issue Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.