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 Rule.oneOf to resolve correct loader #2747

Merged
merged 5 commits into from
Jul 9, 2017
Merged

Conversation

Furizaa
Copy link
Contributor

@Furizaa Furizaa commented Jul 8, 2017

This PR uses the oneOf rule to resolve loaders in webpack.config.prod.js. This will use the first loader that matches in the list of loaders provided by oneOf and enables us to remove the big exclusion list of the file-loader. The file loader will only be used if the module falls through all other loaders.

https://webpack.js.org/configuration/module/#rule-oneof

For some reason though I had to still preserve the js(x) exclusion in the file loader. Otherwise the build would break. This happens with or without the eslint preloader. Will have to investigate further why exactly this happens and why these unknown js(x) modules fall through to the file-loader.

The resolve now looks something like this (pseudocode):

[
  { 'eslint-loader', enforce: 'pre', test: 'javascript' },
  {
    oneOf: [
      { 'url-loader': test: 'images' },
      { 'babel-loader', test: 'javascript' },
      { 'style-loader', test: 'style' },
      { 'file-loader' },
    ]
  }
]

@gaearon
Copy link
Contributor

gaearon commented Jul 8, 2017

This is sooo sweet. Thank you.
Still pending some work?

@Furizaa
Copy link
Contributor Author

Furizaa commented Jul 8, 2017

Thank @sokra as he pointed me in the right direction :D Yeah now I see I missed the dev config. Will add that in a bit.

@gaearon
Copy link
Contributor

gaearon commented Jul 8, 2017

For some reason though I had to still preserve the js(x) exclusion in the file loader. Otherwise the build would break. This happens with or without the eslint preloader. Will have to investigate further why exactly this happens and why these unknown js(x) modules fall through to the file-loader.

Did you figure this part out?

@gaearon gaearon added this to the 1.0.11 milestone Jul 8, 2017
@Furizaa
Copy link
Contributor Author

Furizaa commented Jul 8, 2017

Did you figure this part out?

Not yet. I can try my luck tomorrow but there is a chance I will come to no conclusion. Debugging webpack is a pain and __webpack_require__(...) is not a function does not really help there.

But -- I added oneOf to the dev config. Should be good now if you don't point me to a spot I missed :)

@gaearon
Copy link
Contributor

gaearon commented Jul 8, 2017

Let's hang on until you try to figure it out. If you can't, no sweat, just write here and we'll get somebody familiar with webpack internals to look at it. I'd like to understand better before merging. Thank you!

@Furizaa
Copy link
Contributor Author

Furizaa commented Jul 9, 2017

Ok, I found out what the problem is and have two possible fixes for you. The culprit is the css-loader runtime that gets injected as a .js file that would then be loaded by the file-loader if we don't exclude .js files explicitly ('cause the babel-loader only matches paths.appSrc).

As a proof-of-concept I tried to patch the css-loader so that the runtime gets loaded with the !! prefix which ignores all loaders. But that is another can of worms (weird errors) I was not willing to open.

So here are my proposed solutions:

  • Keep the exclusion and document why it's there.
  • (Cleaner IMHO) Add include: paths.appSrc to the file-loader. But this could potentially break projects that use the file-loader to load files from outside paths.appSrc for whatever reason.

I'm happy to implement either one of these based on feedback.

@gaearon
Copy link
Contributor

gaearon commented Jul 9, 2017

Keep the exclusion and document why it's there.

This sounds good to me.

@Furizaa
Copy link
Contributor Author

Furizaa commented Jul 9, 2017

Should be good now. Thanks for your precious weekend time guiding me through this. If there is anything else I missed then feel free pointing me to it.

@gaearon
Copy link
Contributor

gaearon commented Jul 9, 2017

@EnoahNetzach Let me know if you have any comments here!

@gaearon
Copy link
Contributor

gaearon commented Jul 9, 2017

I’ll get this in for now. Thank you again!

@gaearon gaearon merged commit a08eb3b into facebook:master Jul 9, 2017
romaindso pushed a commit to romaindso/create-react-app that referenced this pull request Jul 10, 2017
* Use oneOf to resolve correct loader

* Add html and json fallthrough again

* Use oneOf to resolve correct loader in dev

* Document file-loaders `js` exclusion

* Remove `jsx` from exclusion in prod config
// it's runtime that would otherwise processed through "file" loader.
// Also exclude `html` and `json` extensions so they get processed
// by webpacks internal loaders.
exclude: [/\.js$/, /\.html$/, /\.json$/],
Copy link

Choose a reason for hiding this comment

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

Instead of the exclude you could also add a empty rule matching these types

{
  test: [/\.js$/, /\.html$/, /\.json$/]
  // Don't use a loader for these types.
}

This would keep the file-loader rule focused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh. Nice! Thanks for that hint. Will create a PR to change this in time. Seems much cleaner.

JohnNilsson referenced this pull request in JohnNilsson/create-react-app-typescript Aug 9, 2017
* Use oneOf to resolve correct loader

* Add html and json fallthrough again

* Use oneOf to resolve correct loader in dev

* Document file-loaders `js` exclusion

* Remove `jsx` from exclusion in prod config
morgs32 pushed a commit to BrickworkSoftware/create-react-app that referenced this pull request Sep 1, 2017
* Use oneOf to resolve correct loader

* Add html and json fallthrough again

* Use oneOf to resolve correct loader in dev

* Document file-loaders `js` exclusion

* Remove `jsx` from exclusion in prod config
JohnNilsson referenced this pull request in JohnNilsson/create-react-app-typescript Sep 9, 2017
* Use oneOf to resolve correct loader

* Add html and json fallthrough again

* Use oneOf to resolve correct loader in dev

* Document file-loaders `js` exclusion

* Remove `jsx` from exclusion in prod config
kasperpeulen pushed a commit to kasperpeulen/create-react-app that referenced this pull request Sep 24, 2017
* Use oneOf to resolve correct loader

* Add html and json fallthrough again

* Use oneOf to resolve correct loader in dev

* Document file-loaders `js` exclusion

* Remove `jsx` from exclusion in prod config
tb added a commit to tb/react-app-rewire-graphql-tag that referenced this pull request Oct 15, 2017
tb added a commit to tb/react-app-rewire-graphql-tag that referenced this pull request Oct 15, 2017
swengorschewski referenced this pull request in swengorschewski/cra-typescript-electron Oct 16, 2017
* Use oneOf to resolve correct loader

* Add html and json fallthrough again

* Use oneOf to resolve correct loader in dev

* Document file-loaders `js` exclusion

* Remove `jsx` from exclusion in prod config
@lock lock bot locked and limited conversation to collaborators Jan 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 this pull request may close these issues.

4 participants