Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

Need for utils/normalizeFallback #144

Closed
michael-ciniawsky opened this issue Aug 16, 2018 · 6 comments · Fixed by #145
Closed

Need for utils/normalizeFallback #144

michael-ciniawsky opened this issue Aug 16, 2018 · 6 comments · Fixed by #145

Comments

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Aug 16, 2018

I think utils/normalizeFallback is not needed and we should revert the logic in src/index.js back to just fallback.call(this, src) since we need to pass all options anyways as it seems e.g

webpack.config.js

{
  loader; 'url-loader',
  options: {
      limit: 1,
      name: '',
      fallback: 'x-loader',
      any: '',
      option: '',
      you: '',
      want: '',
  }
}

should be sufficient

Since #139 all options are again passed to the fallback loader (default && custom), so in case I don't misunderstand the logic in utils/normalizeFallback this change basically neglects all extra 'work' we are currently doing there.

Someone please correct me if I'm wrong 😛

@alexander-akait
Copy link
Member

WIP on this

@Pimm
Copy link
Contributor

Pimm commented Aug 16, 2018

It supports the behaviour proposed/requested in #108 #118.

Essentially, it allows your example to be written in a way which feels more consistent and hygienic to me:

{
  loader: 'url-loader',
  options: {
    limit: 1,
    fallback: {
      loader: 'x-loader',
      options: {
        name: '',
        any: '',
        option: '',
        you: '',
        want: '',
      }
    }
  }
}

As a bonus, it supports the unlikely edge case in which option names would otherwise collide:

{
  loader: 'url-loader',
  options: {
    limit: 1,
    fallback: {
      loader: 'x-loader',
      options: {
        limit: 5
      }
    }
  }
}

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Aug 16, 2018

I see, but then we need to remove the default fallback handling from it and apply it only for a custom fallback since this causes issue(s) with current workflows in conjunction with the file-loader. #139 would need to be 'reverted' aswell then since passing all options was not the original intend of utils/normalizeFallback ?

@Pimm
Copy link
Contributor

Pimm commented Aug 16, 2018

Do you have an example of an issue which is still reproducible after #139?

@alexander-akait
Copy link
Member

Please wait i will send PR in near time

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Aug 16, 2018

? :) Nope it fixed the current issue(s), but I was under the impression that passing all options was not the original intend here (only pass options.fallback.options) ? Or is it mainly about being able to pass an {Object} to options.fallback fallback: { loader: 'x-loader', options: {} }. In this case #139 should be sufficient and it's resolved already (pending release) ?

But let's wait for the PR and continue discussion there then please :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants