Topics/support option list

[sashless]

Be able to use an array for replacements

for #1 (comment)

[sergeylaptev]

Btw i think this way is much better
for single option

{
  search: 'string',
  replace: 'value',
}

for multiple values

{
  multiple: [
    {
      search: 'string',
      replace: 'value',
    },
    {
      search: 'string2',
      replace: 'value2',
    },
  ],
}

The code will look like this:

var utils = require('loader-utils');

function processQuery(source, query) {
  source = source.split(query.search).join(query.replace);
}

module.exports = function(source) {
  this.cacheable();

  var query = utils.parseQuery(this.query);

  if (Array.isArray(query.multiple)) {
    query.multiple.forEach(function(query) {
      processQuery(source, query);
    });
  } else if (typeof query.search !== 'undefined' && typeof query.replace !== 'undefined') {
    processQuery(source, query);
  }

  return source;
};

[sashless]

hm much better because using forEach and a different key ? I would call that just a different approach ;D According to https://jsperf.com/for-vs-foreach/75 forEach is even slower than for loop =)

[sashless]

Oh and your proposal is missing support for regex + flags which would break backward. I prefer to leave this as it is =)

[sergeylaptev]

I said "much better" not for performance, but for structure with/without "multiple" property:)

[sashless]

ah yea okay. This leads me to another thought. Why not doing a regex replacement and in addition array replacement ? Will use your idea with multiple key to do that

[sashless]

ah well nope, dont have enough time to tinker here so long =) will leave it as it is for now

[sergeylaptev]

String#split supports regex as argument, and we don't need "flag" option anymore:) And here is 1.0.0 release:D

[sashless]

ah yea good point !

[sergeylaptev]

And here is the winner;)

var utils = require('loader-utils');

function processQuery(source, query) {
  if (typeof query.search !== 'undefined' && typeof query.replace !== 'undefined') {
    return source.split(query.search).join(query.replace);
  } else {
    return source;
  }
}

module.exports = function(source) {
  this.cacheable();

  var query = utils.parseQuery(this.query);

  if (Array.isArray(query.multiple)) {
    for (var i = 0; i < query.multiple.length; i++) {
      source = processQuery(source, query.multiple[i]);
    }
  } else {
    source = processQuery(source, query);
  }

  return source;
};

[sashless]

ah okay, i did it a bit different but basically the same. I think you have a bug inside your proposal anyway. Afaik source is a string and not passed by reference so you need to return it from processQuery.

[sergeylaptev]

Oops you're right! I've wrote this code in Github, and didn't test it at all. Fixed last proposal

[sashless]

no worries. I didnt test it as well but looks good to me. We will see what @Va1 is saying =)

[sergeylaptev]

Just fixed last bug in processQuery in cases when condition of query properties isn't valid

[sashless]

yea i have seen that as well and already fixed that in my commit https://github.com/easybiblabs/string-replace-loader/commit/adef248908916da6a57be2434a9789759dcb9358 👍

[sergeylaptev]

Good job, the solution is little bit different but main idea is the same:)

[sashless]

yup, i prefer to to return instead of else when possible. Thanks for collaboration =)

[Va1]

Hey, guys!

This all looks fine, I do love the idea of a better interface multiple replacement, I wander why did not I add this feature before.

Though, I am feeling like using source.split(query.search).join(query.replace); is not really cool, because String.prototype.replace() is kinda designed for such operations (+ it supports function as a replacement).

So, I will merge this now, then apply some fixes, then fix the tests, then bump the npm version. Will let you know by posting a message here.

Thanks a lot,
Val

[sergeylaptev]

@Va1 You are welcome;)

[sashless]

oh man sorry, didnt see there are tests available. I would have adjusted them otherwise :/ Thanks for merging !

[Va1]

OK, so. The reason of having flags options and using new RegExp(...) was the fact that webpack seems to be unable to pass a regular expression from a configuration code code to the actual loader code (due to some transformations to string and back on query parsing, I guess). So, when you define a search query option as a RegExp instance, it appears to be {} (empty object) when it comes to replacement. That's why that time I went with passing it as string and using new RegExp(...) with flags there. So now I am force to get back to this solution, while keeping multiple support of course. I wish I bethought of this ugly feature earlier, because I was wandering why tests fail for 20 minutes or so. Unit testing is bliss anyways :)

[Va1]

Done!

[sashless]

nice thanks ! What a pitty that you added lodash again just to check for undefined and array. Can you please remove lodash again ? Those checks are easily done with vanilly js :)