keonik commented 3 years ago

Describe the bug

When trying to import a image using the url method in a css/sass file the path no longer resolves to include the public directory using the prefix /

Did you try recovering your dependencies?

Yes

Which terms did you search for in User Guide?

Environment

  current version of create-react-app: 4.0.0
  running from /Users/jfay/.npm/_npx/12069/lib/node_modules/create-react-app

  System:
    OS: macOS 10.15.7
    CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
  Binaries:
    Node: 12.18.3 - /usr/local/bin/node
    Yarn: 1.21.1 - /usr/local/bin/yarn
    npm: 6.14.6 - /usr/local/bin/npm
  Browsers:
    Chrome: 86.0.4240.75
    Edge: Not Found
    Firefox: 80.0
    Safari: 14.0
  npmPackages:
    react: ^17.0.1 => 17.0.1 
    react-dom: ^17.0.1 => 17.0.1 
    react-scripts: ^4.0.0 => 4.0.0 
  npmGlobalPackages:
    create-react-app: Not Found

Steps to reproduce

Start with a generic cra w/ react-scripts@4.0.0
Open App.css
Set the background to use an image resolved to the public url

.App {
  text-align: center;
  background-image: url("/logo192.png");
}

Expected behavior

Prior to upgrading to react-scripts@latest version 3.4.3 resolved images referenced in css files to include the public directory.

Actual behavior

GersonDias commented 3 years ago

I was about to make report the same issue. Though I'm trying to update from 3.4.3 to 4.0

thomasleduc commented 3 years ago

Same here with scss module. I'm trying to upgrade from 3.4.3 to 4.0

I think I found a related commit by @atlanteh here: https://github.com/facebook/create-react-app/commit/fa648daca1dedd97aec4fa3bae8752c4dcf37e6f

Support scss absolute path resolution for url()

Adding resolve-url-loader broke all apps using scss with centralized assets folder and all url(./assets/.png) broke (#7023). This change allows apps to use url(/assets/.png) and it would map to src/assets/*.png

Does this means that now / refers to the src folder instead of public ? So the size limit to inline image in data url would become irrelevant 🤔 (because, correct me if I'm wrong but I don't think we should serve src)

And what about fonts and other references of file in css ? 🤔

atlanteh commented 3 years ago

@thomasleduc No. My commit fixes an issue I had when using cra with sass after upgrading to cra 3.4.x (don't remember now which version) which caused the exact issue you are describing. The commit causes the loader to correctly load the files during build time from the src folder and copy them to the Public folder. This is very weird. Can you try reverting my commit locally on your machine and see if that's the culprit? If this is in fact the one, then it's worth checking with sass as well and if it works there as well then I guess cra made another fix someplace else and these fixes contradict each other

thomasleduc commented 3 years ago

I use sass module too and this is definitely not working.

I'm glad to do some very little work for create react app, but really the repo needs a proper doc to contribue 😅 I was struggling because yarn pack generate a different .tgz than the one build during the install phase. (react-scripts-v4.0.0.tgz instead of react-scripts-4.0.0.tgz without "v")

So I reverted your commit and I had a different error but it is still here.

With the revert of your commit it has the absolute url /logo192.png :

Without the revert, it has the relative url ../../../../logo192.png.

But I confirm, the error remains present in any case. Thanks for your answer @atlanteh but do you have any clues on how to fix it, it seems you have a good understanding of the resolve-url-loader 😅. I want to give a try to fix the issue because we really needs it to work on our project.

I currently investigate this commit https://github.com/facebook/create-react-app/commit/1cbc6f7db62f78747cb6ca41450099181139325e from multiple authors that I can't just revert. But there are many changes on from where the file are loaded.

kidwm commented 3 years ago

https://github.com/facebook/create-react-app/issues/9833 may be the same bug

atlanteh commented 3 years ago

I just upgraded my project to use cra@4.0.0 and it still works. Are you guys sure you deleted the entire node_modules and reinstalled? Maybe we use things differently? My project setup is as follows:

In my package.json I have "node-sass-chokidar": "1.3.4",
I have src/index.scss file which imports styles from styles directory + ../node_modules/... directories
in index.js I import this file import './index.scss';
I have src/images folder with all kind of images in there
In scss files I'm using these images as follows: background: url(/images/login-bg.png) center/cover no-repeat;

This works for me both in development and in production

Upon reading the issue more thoroughly, I think that the issue is that you are referencing images from public directory, which I guess my commit doesn't allow anymore. Actually I think the new solution is better

Have all your images inside src/images
reference them in the scss files using /images/*.png
only the images actually used will be copied to the build /media folder rather than the entire public directory

Can you please check if this solution work for you?

mgonyan commented 3 years ago

@atlanteh I was facing the same issue described by others above, and as you suggested, moving the assets files used by .scss files from `/public/tosrc/*` fixes the issue (I've migrated from react-scripts@3.4.x to 4.0.0). example:

  .header-bg {
    background-image: url('/assets/images/home-bg-black.png');
  }

This does not work:

public/
   assets/
      images/
         home-bg-black.png

After moving the files to /src:

src/
    assets/
       images/
          home-bg-black.png

Thanks! cc @thomasleduc

GersonDias commented 3 years ago

for me only works after I use relative paths to my images folder (that I moved from public folder). maybe something wrong with my tsconfig?

  "compilerOptions": {
    "outDir": "build/dist",
    "module": "esnext",
    "target": "ES2015",
    "downlevelIteration": true,
    "lib": [
      "es6",
      "dom",
      "es2018"
    ],
    "sourceMap": true,
    "allowJs": true,
    "jsx": "react",
    "noFallthroughCasesInSwitch": true,
    "moduleResolution": "node",
    "rootDir": "src",
    "forceConsistentCasingInFileNames": true,
    "noImplicitReturns": true,
    "noImplicitThis": true,
    "noImplicitAny": false,
    "strictNullChecks": false,
    "suppressImplicitAnyIndexErrors": true,
    "noUnusedLocals": false,
    "allowSyntheticDefaultImports": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "strict": true,
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "experimentalDecorators": true,
    "baseUrl": "src"
  },
  "include": [
    "src"
  ]
}

also I needed to include the noFallthoughtCasesInSwitch: true and changed the key the jsx from preserve to react

keonik commented 3 years ago

@mgonyan if the fix is to move all assets into the src folder that could be as simple as updating the docs to list that as a breaking change. Also updating the docs here

thomasleduc commented 3 years ago

if the fix is to move all assets into the src folder that could be as simple as updating the docs to list that as a breaking change. Also updating the docs here

Maybe change the template as well to add some image in src/assets/images

also I needed to include the noFallthoughtCasesInSwitch: true and changed the key the jsx from preserve to react

This is already fixed on master. I'm afraid we have to wait for the next release. ¯_(ツ)_/¯ Does someone knows how to refer to master or an unpublished work on react-scripts ?

Anyway, Thanks a lot for your time @atlanteh. It is greatly appreciated 👍

makis-spy commented 3 years ago

I thought I was going crazy. 5 mins to get the app running 2 hours of googling to fix this issue :/

Also moving images inside source is not a solution if you want common inline and sass referenced images without duplicating them

atlanteh commented 3 years ago

@makis-spy First of all, I'm very sorry that you had to go to all this trouble. I guess it's on me. I always used the images in the src and things stopped working for me (and others) when trying to update to 3.4, so I found that solution and made a PR. Regarding your concern, can you please elaborate on that? What do you mean by "common inline images"? If the images are small enough, then they will be inlined for you automatically. I'm not sure I understand what difference changing the folder makes..

makis-spy commented 3 years ago

@atlanteh oh no worries mate!

Maybe you can shed some light. So I have a public/img folder and a src/img folder

< img src="/img/me.png" /> calls public/img

.avatar { background:url(//img/me.png) calls src/img }

Do I need 2 images of me ?

What I am finding works for anyone needing a quick fix is keeping 2 sets of images (ducks a tomato)

One in public for after rendering

One in source just to bypass the file not found by loader

atlanteh commented 3 years ago

I think your < img src="/img/me.png" /> is wrong. You need to do it this way:


import MyImage from 'src/img/me.png';
...
function MyAvatar() {
  return (<img src={MyImage} />)
}

thomasleduc commented 3 years ago

@atlanteh If I use an image very often, say a logo that I put everywhere. It seems to me that inline this logo is not the solution even if it's small.

Say I use it 50 times in page, if I fetch the image, it would be cached in browser and reused across the all page. If instead I inline it, my html bundle would be very huge. I don't know if gzip would do the work on the bundle since there is a repetition; But it looks not good to me.

I think the developer should have the choice to make a http call or inline the image. don't you think ?

Again, it is not your fault, and I'm glad you're here to help :)

atlanteh commented 3 years ago

@thomasleduc If you import this image in a Logo component and use that component everywhere, I think everything should be ok. Since this image is only imported once, and only the Logo reference is being used everywhere, so the bundle should not be affected by the times the Logo is used. If that's not an option, you can always disable image inlining by setting env IMAGE_INLINE_SIZE_LIMIT=0:

https://create-react-app.dev/docs/adding-images-fonts-and-files/

To reduce the number of requests to the server, importing images that are less than 10,000 bytes returns a data URI instead of a path. This applies to the following file extensions: bmp, gif, jpg, jpeg, and png. SVG files are excluded due to #1153. You can control the 10,000 byte threshold by setting the IMAGE_INLINE_SIZE_LIMIT environment variable as documented in our advanced configuration.

thomasleduc commented 3 years ago

Ohh ok, I didn't think it this way. Thanks a lot for all your answers and for your work @atlanteh 🙏

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

sagar7993 commented 3 years ago

I'm surprised that more people haven't realised that this is a major concern for SEO.

Lets say that you have a image file within the src folder, and you're using it within your CSS file like this -

.Profile { text-align: center; background-image: url("../../images/user/profile.png"); }

If this profile.png file is inside the src > images > user directory, then while creating a build with react-scripts, the URL generated for this file will be something like this -

https://{my-domain.com}/static/media/profile.{hash}.jpg

This {hash} value changes with every build. Therefore, if you use this in a static website, and if your website is crawled by google bot, you may see 404's reported in the google search console the next time you deploy, because Google can take several weeks before indexing your site again.

This is where an image in the public folder would have helped, because that URL will never change, and can safely remain indexed in search engines.

hershkoy commented 3 years ago

I also had this issue this week. https://stackoverflow.com/questions/65603121/react-prevent-images-change-name-on-build-in-css?noredirect=1#comment115988721_65603121

And I agree about the SEO comment from @sagar7993

jsdev-mario commented 3 years ago

same issue

itsmatin commented 3 years ago

Been dealing with the same issue for couple of hours now. Uninstalling and reinstalling node_modules didn't do much...

atlanteh commented 3 years ago

@sagar7993 @hershkoy @itsmatin CRA adds hash of the content of the file, so as long as the file remains the same, the hash should not change. I just verified this on a clean CRA installation and on a production app I'm using. Both produce the same files hashes across builds as long as the files don't change. If any of you is getting a different results, can you please share?

zirklutes commented 3 years ago

I am experiencing the same issue. I am using 17.0.1 react version. Is there a workaround to reach public folder inside css file?

ccheatham commented 3 years ago

same issue

vortegon commented 3 years ago

same issue

luisFebro commented 3 years ago

it seems like this issue is happening only for relative paths inside (s)css-based files. All other paths are working fine apparently. Luckily I had only a few files with these relative paths and after changing to src folder, the issue is gone.

ghost commented 3 years ago

Having the same issue, I also think that moving the images folder into the src folder is not a good practice. Will this be addressed? Thanks.

tray2100 commented 3 years ago

I hate to be the "+1" or "I'm broken too" guy but ... yeah. This seems like quite a monumental change for a lot of people and probably should have been controlled by some configuration flag where this new behavior could have been optionally adopted.... :(

f1am3d commented 3 years ago

Same issue.

xbrunosousa commented 3 years ago

Same here

amiranga commented 3 years ago

Same issue.

mchl18 commented 3 years ago

I am sorry but it is outrageous that such a basic thing is not working in the number one frontend framework. Should be able to use simple absolute string paths for many reasons, no matter if in HTML or CSS or JS.

All I am trying to do is to inject an SVG icon into a d3 graph dynamically (no JSX involved) and at least locally I cannot find a path that works.

ccheatham commented 3 years ago

I am sorry but it is outrageous that such a basic thing is not working in the number one frontend framework. Should be able to use simple absolute string paths for many reasons, no matter if in HTML or CSS or JS

Not sure outrage is the best position... it's FOSS...become a contributor and technically you could fix it yourself.

atlanteh commented 3 years ago

First I'd like to apologize as my commit broke this for everyone. My commit fixed an issue where absolute path resolution didn't work in scss.
Honestly the fix for 99% of the users is very simple and I don't think this change is going to get reverted back as this is a good change.. I totally agree that this should have been clarified and declared, but I never used images in public directory nor were there any tests for files being used in public directory that could fail the build, so this was missed.
My opinion is that putting files in public directory is not the intended use case. images should be stored in src, referenced in code and only those actually used, will be copied to the build directory. I believe this is a good change, which doesn't really have any drawbacks (other than requiring people to move the images & adjust the code), so I think the best & fastest solution to your problem is to just fix the code. Again sorry for the issue this has caused you guys, and if you have a scenario where you think this change causes real trouble, please let me know and we can discuss this and try to find a valid solution.

mchl18 commented 3 years ago

@atlanteh I am very sorry for my harsh overreacted comment. I have only worked on a React project for the third time now and all were quiet brief but this is something that always needed some kind of special treatment and I just find it rather weird that we need to spend way too much time figuring out path resolution for assets which IMHO should be a given. You are not to blame here.

tray2100 commented 3 years ago

As I mentioned before in this thread a change like this is not inherently wrong. However since it fundamental changes existing behavior, it should be gated with a flag so that users can optionally opt into the new behavior versus being forced into it.

There could be immense code bases that may require months of effort to transition images to be under the src/ folder and have quality assurance performed just to ensure that the transition didn't break/distort anything.

Its on my list of things to do to go through this commit and wrap it with a flag but I just haven't had the time. If someone has the bandwidth, please feel free to contribute!

@atlanteh if you have the bandwidth to do this, it would help a lot of people out. 😀

On Wed, Jun 2, 2021, 4:10 PM mchl18 @.***> wrote:

@atlanteh https://github.com/atlanteh I am very sorry for my harsh overreacted comment. I have only worked on a React project for the third time now and all were quiet brief but this is something that always needed some kind of special treatment and I just find it rather weird that we need to spend way too much time figuring out path resolution for assets which IMHO should be a given. You are not to blame here.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/facebook/create-react-app/issues/9937#issuecomment-853440366, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIQRRYEBVKD2ELF6AF5QVTTQ225NANCNFSM4TCSGBPA .

atlanteh commented 3 years ago

@tray2100 while I agree that this is a breaking change that should not have been forced onto users, I doubt the CRA team will accept any prs for adding flags for several reasons:

CRA is aimed to be 0 configuration boilerplate, and adding a flag for such a small userbase is very unlikely.
Adding images to public directory directly is not the intended behavior and it should be discouraged
There isn't real business scenario (from what I saw people have specified in the comments) forcing people to keep the images in public directory
The change was done in migration from CRA 3 -> CRA 4 so breaking changes are acceptable
You can easily revert to the previous CRA and keep working with it until you decide to upgrade
The userbase that require this change is very small.. few dozens top, and adding the flag, documentation and testing while most of them can overcome this issue in a few minutes is just not worth it

The way I see it these are the options we have:

Upgrade CRA and Move your images into src folder (Should take no longer than several minutes to apply for medium projects)
Keep using old CRA until you have the capacity to upgrade
Upgrade CRA, eject and update the flag on your own. (You want be able to get automatic new CRA updates)

Again, sorry this was missed in the breaking changes... waiting to see how many downvotes I'm going to get :)

perjacks commented 3 years ago

How would you go about dynamically importing images from src-folder based on runtime user interaction - i.e. 1.jpg, 2.jpg, 3.jpg etc? In my experience this can only be done if the images reside in the public folder...?

atlanteh commented 3 years ago

@perjacks that depends on your usecase;

This is a limited set of images used to represent the status of an item:


import ActiveImage from 'src/img/active.png';
import DeletedImage from 'src/img/deleted.png';
import ArchivedImage from 'src/img/archived.png';

function ItemStatus({status}) { let img = ''; swtich (status) { case 'active': img = ActiveImage; break; ... } return () }



2. This is a dynamic image like user profiles/product images - in this case, the images should be stored in an external storage (aws/gcp) and not be bundled as part of your app

perjacks commented 3 years ago

Okay, say I have a png-sequence of 1500 images that I need to load and play back. Not really practical to add 1500 import statements to the code. This would be simple to do with images in the public folder - not in the src. And why use build ressources on copying images from src to public if they are already optimized once and for all?

Putting images on external sources is not always an option and shouldn’t be a dependency in my opinion.

I think this is a pretty common scenario that may have been overlooked.

atlanteh commented 3 years ago

I might be mistaken, but I believe your scenario has nothing to do with the main issue in this thread. This thread focuses on css not being able to reference images in public folder. In your case, is setting up 1500 different css classes better than 1500 imports? What I would do if I were you, would be to put all images in public folder, and reference them by path in your img src like so: <img src={`/myImg-${index}.png`} /> This is obviously still possible and I believe the best fit for your scenario. Note that you won't know in build time if one of your images is missing, but I believe that if this is some closed set that never changes, this might be fine.

mustafa519 commented 3 years ago

I have to eject CRA all because of this issue. So it's not user-friendly and not good for newcomers, I think... I also believe that's not good to force people to use like that even you have a good reasons for that. I was spent a lot of time to fix this and wouldn't know it has changed until find this issue.

Furthermore, I also believe it increases the build time if the project contains too many files. So this change is not good for me in any case. Why we just can't set an option in ENV files for this? Or something like that. It would be better not to do that kind of the changes until decide it with the community together. So, I think, sometimes it's better to bring a flexible option for the community.

maciejgaciarz commented 3 years ago

For quick fix I already posted this in couple of places

Hey for quick fix you can use craco (https://github.com/gsoft-inc/craco/tree/master/packages/craco)

install per instructions. In my case im using .cracorc file.

Put this inside .cracorc: module.exports = { reactScriptsVersion: "react-scripts", style: { css: { loaderOptions: () => { return { url: false } } } } } Save, when launched with changed package.json (see craco install instructions) with CRA 4.0 .scss properly resolves from public just like before

iamart commented 3 years ago

@atlanteh the problem is we have to have 2 image folders in src and public, which is ridiculous.

if we use only public - then we can't use url() in css; so we need to store images in src. if we use only src - then we can't do < img src="/img/me.png" />, so we have to import every single image. Tsx/jsx already makes component files long by having logic & styling together but okay, then we want to make it responsive and we add srcSet. We can't feed an import into srcSet, so we still need duplicates in public anyway.

So in the end what I hear from you is I need a dozen of imports and two image folders now.

elixiao commented 3 years ago

This behavior is ridiculous. url('/xxx') should refer to public/xxx

bruncun commented 3 years ago

@maciejgaciarz's fix worked for me until I built for prod because the leading backslash of my URLs was being removed. To resolve this, I further configured craco to remove resolve-url-loader and add it back without the newly-introduced root option.

craco.config.js

const { loaderByName, addAfterLoader, removeLoaders } = require("@craco/craco");

module.exports = {
  reactScriptsVersion: "react-scripts",
  style: {
    css: {
      loaderOptions: () => {
        return { url: false };
      },
    },
  },
  webpack: {
    configure: (webpackConfig, { env }) => {
      const isEnvDevelopment = env === "development";
      const isEnvProduction = env === "production";
      const shouldUseSourceMap = process.env.GENERATE_SOURCEMAP !== "false";
      removeLoaders(webpackConfig, loaderByName("resolve-url-loader"));

      const resolveUrlLoader = {
        loader: require.resolve("resolve-url-loader"),
        options: {
          sourceMap: isEnvProduction ? shouldUseSourceMap : isEnvDevelopment,
        },
      };

      addAfterLoader(
        webpackConfig,
        loaderByName("postcss-loader"),
        resolveUrlLoader
      );

      return webpackConfig;
    },
  },
};

sagar7993 commented 3 years ago

@sagar7993 @hershkoy @itsmatin CRA adds hash of the content of the file, so as long as the file remains the same, the hash should not change. I just verified this on a clean CRA installation and on a production app I'm using. Both produce the same files hashes across builds as long as the files don't change. If any of you is getting a different results, can you please share?

@atlanteh You're right, CRA won't change the hash after build until the file has changed. The problem I was trying to explain is that if you change the file and deploy the build, the previous url will no longer be available.

If the file was inside the public folder, then the hash wouldn't have mattered. I can safely update the file, and the url would continue to work and the search engine crawlers would pickup the new file without facing any 404's.

atlanteh commented 3 years ago

@sagar7993 Yes, this makes total sense. To be honest, to me (and I'm not affiliated with CRA at all, and have no research about it), CRA feels very much suitable for inside apps, rather than business front websites that are meant to be crawled by search engines. I know search engines are getting better and better at reading SPAs, but in general, it seems to me that for front facing websites, it makes more sense to use SSR frameworks, likely with CMS built in, plugins & builders (like Wordpress etc), and you can see that for the past year, only 65 people bothered enough to like this issue, and even less commented about it.

BTW, for all you guys, please note that my commit fixes another backward compatibility break that adding url loader introduced (#7023) . Instead of complaining and lashing off at me/CRA team, you can do what I & other developers do. If this issue is so important to you, you are welcome to do some research find a suitable solution, open a discussion about it (in another issue), explain the cons & pros and why it's better then the current solution, nudge CRA team if you believe your issue doesn't get enough attention and try to make CRA just a little bit better. Adding angry comments to seemingly untracked issue is not going to help you achieve what you're after.

zpetukhov commented 3 years ago

Fixing a backward compatibility break by adding another breaking change... awesome. A lot of people were affected. A lot of person-hours were spent to resolve the consequences. Only a little portion of affected developers even saw this ticket. If you are doing so, you get a bunch of dislikes, it's to be expected. No matter you are not affiliated with CRA. That's OK. It's your choice to fix that or not, you did the choice to not. That's also OK. Further justifications looks unnecessary and inappropriate.

only 65 people bothered enough to like this issue

There are some duplicate issues also. #9870 for example. if you sort by "most commented" or by "most reactions", both the issues are on the first page. That's an indicator that a lot was affected, isn't it.

7023 (the cause of the changes) has a lot less likes and comments.

This one seems also related (correct me if I am wrong): https://github.com/webpack-contrib/css-loader/issues/1256

facebook / create-react-app

public directory css url #9937

Describe the bug

Did you try recovering your dependencies?

Which terms did you search for in User Guide?

Environment

Steps to reproduce

Expected behavior

Actual behavior

7023 (the cause of the changes) has a lot less likes and comments.