RFC: Pass the filename as the 2nd argument to the to function (shift remaining existing arguments by 1 to the right)

[gregmagolan]

I found I needed the filename to determine the correct replacement string for my use case so I added the filename as the 2nd parameter of the to replacement function (if it is of type 'function').

The change is straight forward. I haven't added any tests to .spec for it yet but thought I would check if you would be interested in merging the feature into the library before bothering.

It would break backward compatibility to anyone using more than the first argument in a to replacement function (which are passed in by the contents.replace(item, replacement) call).

[coveralls]

Coverage decreased (-1.0%) to 99.022% when pulling dc9e4bc on gregmagolan:feature/pass-filename-to-to-function into c044c5a on adamreisnz:master.

[coveralls]

Coverage decreased (-1.5%) to 98.54% when pulling 3472274 on gregmagolan:feature/pass-filename-to-to-function into c044c5a on adamreisnz:master.

[adamreisnz]

@gregmagolan Hmm, I wonder if we could make this without being backwards breaking, for example making the filename (and possibly other parameters) available through the this context of the replacement function.

That way the order of arguments would remain untouched, while making the filename accessible.

Thoughts?

[adamreisnz]

Why this second check (already checked on line 102)?

[gregmagolan]

Oh yeah. Typo. Will change that.

[coveralls]

Coverage decreased (-1.2%) to 98.78% when pulling 5dc1d62 on gregmagolan:feature/pass-filename-to-to-function into c044c5a on adamreisnz:master.

[coveralls]

Coverage decreased (-1.2%) to 98.78% when pulling 5dc1d62 on gregmagolan:feature/pass-filename-to-to-function into c044c5a on adamreisnz:master.

[gregmagolan]

Having the filename available through this would force the usage of ES5 function (match) { ... } wouldn't it? If you use an ES6 arrow function within an ES6 class like so:

class Something {
  replaceStuff() {
    replace.sync({
      files: [ ... ],
      from: 'foobar',
      to: (match) => { ... }
    });
  }
}

this would be the class context that the arrow function is in.

[adamreisnz]

True, in that case, how about always passing it in as the last parameter?

[gregmagolan]

I thought of that as well but the number of parameters will be variable based on the number of groups in the regex expression when using regex: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace.

Usage would be more difficult in that case and you'd have to use arguments instead of function parameters when dealing with variable parameters.

[gregmagolan]

What would be nice is getting the groups as an array in the replacer function instead of as parameters if backward compatability is going to break anyway. That way the number of parameters is fixed like so:

to: (match: string, filename: string, groups: string[], offset: number, wholeString: string) => {
  ...
}

(using typescript typing above to annotate the expected types)

[adamreisnz]

That's an option, but instead of changing the function signature you could use the rest operator to get what you need, even with variable arguments;

to: (match, ...args) => {
  const file = args.pop();
  ...
}

I'm preparing a 3.0.0 branch regardless, so we can include this in there without worrying about backwards compatibility too much.

[adamreisnz]

@gregmagolan Could you open a new PR for the 3.0.0 branch based on that code base?

If you could pass the file parameter as the last argument, I think that would be the best solution, as it's the least chance of backwards breaking implementations and it will leave the signature of the function mostly intact, save adding a new param. I think with the rest operator to capture args it's easy enough to get the last one.

Feel free to write the tests as well to keep coverage at 100% and I'll merge it in before releasing 3.0.0 👍

[gregmagolan]

I hadn't thought of using the rest operator like that. That would work well with the file as the last parameter. I'll prepare another PR on the 3.0.0 branch :)

[adamreisnz]

Awesome thanks!