refactor(index): remove unnecessary delete

[alexander-akait]

This PR contains a:

• bugfix
• new feature
• code refactor
• test update
• typo fix
• metadata update

Motivation / Use-Case

we don't support webpack@3 in this package

Breaking Changes

no

Additional Info

delete is very expensive operation (perf)

/cc @insin can you test this branch to avoid we fix problem #138

[alexander-akait]

@michael-ciniawsky don't merge before @insin until it tested

[alexander-akait]

@michael-ciniawsky looks we do breaking change here https://github.com/webpack-contrib/url-loader/blob/master/src/utils/normalizeFallback.js#L15 we don't pass limit, mimetype options to fallback loader, but here https://github.com/webpack-contrib/url-loader/blob/master/src/utils/normalizeFallback.js#L45 do. We need fix it.

We should pass all option(s) from url-loader to fallback loader or add fallbackOptions option for fallback options and don't mix options. Example i want use one name for file-loader and other name for responsive-loader. What do you think? Maybe better solve this right now and release major than to create problems in the future for us.

[michael-ciniawsky]

Can't we just accept an {Object} for options.fallback ?

{
   ...
   use: { 
      loader: 'url-loader', 
      options: {
         fallback: { 
           loader: 'responsive-loader', 
           options: { ... } // We only pass these to the custom fallback
         }
      }
   }
}

and for the default (file-loader) we pass the options like we did before ? If the fallback is a {String} we don't pass any options relying on the custom fallback loader defaults ?

[alexander-akait]

Good idea, let' do it, if no options defined we use url-options. Should options inherit? I.e. if i defined name for url loader, should we pass name to fallback? I think yes

[michael-ciniawsky]

We can do that yes, but I think we should 'cherrypick'/limit them to only the useful/required ones (e.g options.name) and not to all by default for a custom fallback loader. For the file-loader (default) we pass all of the url-loader options like before

url-loader (Inline)

webpack.config.js

{
  loader: 'url-loader'
  options: {
     name: '...',
     limit: 10000
  }
}

file-loader (fallback [default])

webpack.config.js

{
  loader: 'url-loader'
  options: {
     name: '...',
     ...,
     limit: 10
  }
}

x-loader (fallback {String} [no fallback options])

webpack.config.js

{
  loader: 'url-loader'
  options: {
     name: '...',
     limit: 10,
     fallback: 'x-loader'
  }
}

x-loader (fallback {Object} [with fallback options])

webpack.config.js

{
  loader: 'url-loader'
  options: {
     name: '...',
     limit: 10,
     // fallback.options merged with e.g options.name if typeof options.fallback === 'object'
     // and called afterwards
     fallback: { loader: 'x-loader', options: { ...custom } }
  }
}

[michael-ciniawsky]

if that works (didn't tried/tested yet :))

[alexander-akait]

@michael-ciniawsky what about const fallbackOption = Object.assign({}, urlOptions, fallbackOptions)?

1. If no fallback - we pass url-loader options to file-loader/x-loader.
2. If fallback string - we parse query string, merge their with url-loader options and pass to file-loader/x-loader - const fallbackOption = Object.assign({}, urlOptions, fallbackOptions).
3. If fallback object - we get fallback options and merge their with url-loader options and pass to file-loader/x-loader - const fallbackOption = Object.assign({}, urlOptions, fallbackOptions).

We should always pass all options, but fallback options always rewrite url-loader options (example name from fallback.options always rewrite name from url-loader, same for limit). Also we should pass all options, because fallback loader also can use name/limit options.

[michael-ciniawsky]

Wouldn't that mean to just use fallback.call(this, src) (L62) (this === loaderContext) again, passing all options ?

{
  loader: 'url-loader'
  options: {
    name: '[name].[ext]',
    fallback: 'x-loader',
    'x-option1': '',
    'x-option2': ''
  }
}

[alexander-akait]

@michael-ciniawsky yep, just merge options: Object.assign({}, urlOptions || {}, fallbackOptions || {}). Universal way to pass all options and prefer fallback options under url-loader options

[michael-ciniawsky]

Yes, but if the custom fallback loader options are declared on the url-loader options we can even avoid the need to merge them since all options are already provided and added to the loaderContext as query (?) What was the reason to add the utils/normalizeFallback 'hack' ?

[alexander-akait]

@michael-ciniawsky for backwards compatibility since we in previous versions sent all the options, we can add comments about remove this in 2 version

[michael-ciniawsky]

What do you mean by removing this (?) in version 2.0.0 :). I'm a bit confused by the utils/normalizeFallback in general (Why do we need it ?), but might be missing something. If we pass all options it should be sufficient to 'revert' to fallback.call(this, src) in src/index.js and in your webpack.config.js one simply uses

webpack.config.js

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

like it was before, but there must be a reason why utils/normalizeFallback was added in the first place, but I don't understand 😛

[alexander-akait]

@michael-ciniawsky i will send new PR and we continue the discusion in it, this PR we can merge

[michael-ciniawsky]

Does #139 💯 fixed it for now ? I can open an issue and we can discuss separately since people seem to have quite some issues with we better release a patch asap :)

[michael-ciniawsky]

:D you're faster, yep agreed 👍