adamreisnz / replace-in-file

A simple utility to quickly replace contents in one or more files
580 stars 65 forks source link

fix(dts): adapt ReplaceInFileConfig interface to actual implementation #107

Closed antongolub closed 4 years ago

coveralls commented 4 years ago

Coverage Status

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

antongolub commented 4 years ago

@adamreisnz could you release the fix?

adamreisnz commented 4 years ago

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

kvpt commented 4 years ago

@antongolub I tested it.

For me it's cause this issue :

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

antongolub commented 4 years ago

@kvpt

What TS version was used?

kvpt commented 4 years ago

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

kvpt commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@adamreisnz, @gotik, @JonnySpruce, @kvpt

Any objections or suggestions?

kvpt commented 4 years ago

@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: https://github.com/adamreisnz/replace-in-file/blob/22364d3540a4df7482b6767d342c1aedcb952373/types/index.d.ts#L3

Here: https://github.com/adamreisnz/replace-in-file/blob/22364d3540a4df7482b6767d342c1aedcb952373/types/index.d.ts#L4

And here: https://github.com/adamreisnz/replace-in-file/blob/22364d3540a4df7482b6767d342c1aedcb952373/types/index.d.ts#L7

antongolub commented 4 years ago

@kvpt could you verify the last improvement?

kvpt commented 4 years ago

@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 commented 4 years ago

I'd like to keep namespace for backward compatibility.

antongolub commented 4 years ago

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

adamreisnz commented 4 years ago

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 commented 4 years ago

@adamreisnz, could anyone else check this out?

antongolub commented 4 years ago

@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 commented 4 years ago

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

antongolub commented 4 years ago

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

adamreisnz commented 4 years ago

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

adamreisnz commented 4 years ago

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