Skip to content

fix: disable esModule on file-loader and url-loader to fix require() issues #9934

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

Closed
wants to merge 1 commit into from

Conversation

n3tr
Copy link
Contributor

@n3tr n3tr commented Oct 28, 2020

In #8950,

There is a breaking change in file-loader and url-loader (esModule enable by default). This causes require(..) returns an object instead of a string, and it will break projects that upgrade from v3 to v4.

This PR will disable esModule for those loaders to preserve the require behavior as in v3

Verify step

Both statements below should work.

const image = require('./lmage.png');
import image from './image.png';

I don't know if it is an intention to enable esModule in the loader or not. If so, we probably need to add it as a breaking break of v4 as well.


fix #9721, fix #9831

@n3tr n3tr changed the title fix: disable esModule on file-loader and url-loader fix: disable esModule on file-loader and url-loader to fix require() issues Nov 3, 2020
Copy link
Contributor

@brunolemos brunolemos left a comment

Choose a reason for hiding this comment

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

If you pass esModule: false, does import image from './image.png' keep working? If not, maybe we should leave ESModule as default and just mention the breaking change in the release description.

People using require can change to require('./image.png').default or import image from './image.png'.

@n3tr
Copy link
Contributor Author

n3tr commented Nov 3, 2020

@brunolemos

If you pass esModule: false, does import image from './image.png' keep working? If so, fine, if not, maybe we should leave it as is and just add this breaking change to the release description; people can use require('./image.png).default instead.

I have tested and both require and import statements are working. This is actually the behavior in v3 (esModule: false). :)

Copy link
Contributor

@brunolemos brunolemos left a comment

Choose a reason for hiding this comment

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

Tested and it works.

@brunolemos
Copy link
Contributor

Since @ianschmitz has created the issue #8355, he may have some opinions on this.

@ianschmitz
Copy link
Contributor

I'm not sure we'll want to move backwards to disable esModule as it really is the future (improves module concat and tree shaking abilities in webpack).

I don't believe we've ever recommended or suggested using require() to import images (see https://create-react-app.dev/docs/adding-images-fonts-and-files). Although i agree we probably should have mentioned in the release notes.

@ianschmitz ianschmitz added this to the 4.0.1 milestone Nov 10, 2020
@bgmort
Copy link

bgmort commented Nov 12, 2020

If nothing else, a note should at least be added to the changelog about this.

@iansu iansu modified the milestones: 4.0.1, 4.0.2 Nov 23, 2020
@iRoachie
Copy link
Contributor

iRoachie commented Nov 29, 2020

I don't believe we've ever recommended or suggested using require() to import images (see https://create-react-app.dev/docs/adding-images-fonts-and-files).

@ianschmitz Do you have any thought about re-considering this? This has been a very convenient way of working with images in react. Imagine a page/component that uses 10 images, and then we have to create import statements for each of these 👎 .

As for developer experience, this ain't it 😞 . Inline require are much easier and clearer to use here.

@ianschmitz
Copy link
Contributor

I don't understand the developer experience issue. The difference should only be if you were using require(), you'll have to add .default to the end of the statement. How is import not working for you?

@iRoachie
Copy link
Contributor

Oh no, I was just referring to when you said the docs don't recommend use of require. Just pointing out it's a pretty useful, and common pattern as opposed to an import statement for every asset. So even though it's not stated, it's very much used.

@mrmckeb
Copy link
Contributor

mrmckeb commented May 31, 2021

I was chatting to @ianschmitz about this and we've decided to close this for now as we don't recommend using require, and there is a small workaround (adding .default) for anyone not wanting to move to ES imports.

Apologies that this was missed in the migration notes from v3 to v4, we understand that must have been frustrating for some people.

@mrmckeb mrmckeb closed this May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v4] Using require to import an image results in an object [CRA4] require doesn't work with images
8 participants