fix(dts): adapt ReplaceInFileConfig interface to actual implementation

[antongolub]

No description provided.

[coveralls]

Coverage remained the same at 100.0% when pulling 531b1ec on antongolub:master into 2a96bb2 on adamreisnz:master.

[antongolub]

@adamreisnz could you release the fix?

[adamreisnz]

I'll need a second opinion on this from another TS contributor.
@Gotik, @JonnySpruce care to comment/review?

[kvpt]

@antongolub I tested it.

For me it's cause this issue :

error TS2303: Circular definition of import alias 'sync'
L10 export { sync };

[antongolub]

@kvpt

What TS version was used?

[kvpt]

@antongolub I used the version 3.6.4, updating to 3.7.5 fixed the issue.

[kvpt]

While we are modifying it, I think there are others issues in this type definition.

I want to be able to do this :

import { replaceInFile , ReplaceInFileConfig, ReplaceResult } from 'replace-in-file';

...

const options: ReplaceInFileConfig = {
    files: filePath,
    from: regex,
    to: replaceRegex
};

const results: ReplaceResult[] = await replaceInFile(options);

//or 

const results: ReplaceResult[] = replaceInFile.sync(options);

...

But it is not possible because of missing exports.

The two functions replaceInFile need to be exported.
The namespace replaceInFile need to be exported.

In this case the sync export you added is unnecessary.

[antongolub]

@kvpt

I've added duplicate sync declaration to be able to use named import like this:

import {sync as replaceSync} from 'replace-in-file'

This helps to avoid imperative renaming.

const replaceSync = replaceInFile.sync

[antongolub]

@adamreisnz, @Gotik, @JonnySpruce, @kvpt

Any objections or suggestions?

[kvpt]

@antongolub Your modifications work fine for me, and with your latest changes also with older typescript version 👍

Like I said, I think we should also add missing exports

Here:

replace-in-file/types/index.d.ts

Line 3 in 22364d3

function replaceInFile(config: ReplaceInFileConfig): Promise<ReplaceResult[]>;

Here:

replace-in-file/types/index.d.ts

Line 4 in 22364d3

function replaceInFile(config: ReplaceInFileConfig, cb: (error: Error, results: ReplaceResult[]) => void): void;

And here:

replace-in-file/types/index.d.ts

Line 7 in 22364d3

namespace replaceInFile {

[antongolub]

@kvpt could you verify the last improvement?

[kvpt]

@antongolub Your changes are good for me.
The only question I have is the purpose of the replaceInFile namespace, now that all the functions inside are hoisted. For me it is now useless and without being exported we can't access methods inside, or I miss something ?

[antongolub]

I'd like to keep namespace for backward compatibility.

[antongolub]

@adamreisnz, @Gotik, @JonnySpruce, any feedback is appreciated.

[adamreisnz]

I don't get involved on the TypeScript side of things, so will defer to the others. If they approve the changes, I'm happy to merge.

[antongolub]

@adamreisnz, could anyone else check this out?

[antongolub]

@adamreisnz, could you summon other reviewers? I can also add some dtslint-based tests for typings, but it requires additional dev deps. If it's suitable, let me know.

[adamreisnz]

@Gotik @JonnySpruce @kvpt would you mind having one last look at these changes before they are merged? 💯

[antongolub]

@adamreisnz, no feedback = no objections. Let's merge.

[JonnySpruce]

Apologies for the delay - fair warning I haven't worked with TypeScript since July so I may be a bit rusty, but since nobody else has looked at this I thought I'd do what I can to make sure the work does get merged.

I think the changes look good, they're definitely all improvements over the previous code. There are some minor non-Typscript changes to the way the module exported so you may want to make sure you're happy with that @adamreisnz. (FWIW I think they're good improvements, and appear to be backwards compatible).

[JonnySpruce]

Would this change not also suggest an Array of FromCallback type could happen?

[antongolub]

It's possible, but let's inspect the current implementation:

1. replaceInFileSync invokes parseConfig
2. parseConfig optionally handles files and ignore fields, injects defaultConfig and returns it back

Object.assign({}, defaults, config)

3. Then config params are passed to replaceSync

const {files, from, to, dry, verbose} = config;
...
paths.map(path => replaceSync(path, from, to, config));

4. and then to makeReplacements

makeReplacements(
    contents, from, to, file, countMatches
  );

5. where from turns into array

  if (!Array.isArray(from)) {
    from = [from];
  }

6. which is reduced

const newContents = from.reduce((contents, item, i) => {
  // ...
    if (typeof item === 'function') {
      item = item(file);
    }
  // ...
}

It looks that Array<FromCallback> is supported.

[JonnySpruce]

What's the benefit of having both sync and replaceInFileSync?

[antongolub]

sync is just a legacy alias for backward compatibility
New named exports let us to avoid using of default module reference:

// Before
import replaceInFile, {sync as replaceInFileSync} from 'replace-in-file'
// After
import {replaceInFile, replaceInFileSync} from 'replace-in-file'

[JonnySpruce]

LGTM - changes are definitely an improvement.

[adamreisnz]

Great, many thanks @JonnySpruce in helping get this over the line

[adamreisnz]

I am a bit surprised that this works and passes the travis build in Node versions 8 and 10?
I thought ES6 style import were only available in Node from version 12 onwards.

[adamreisnz]

Other than the comment above which would be good to clarify, I think we're ready to merge