lazd / gulp-replace

A string replace plugin for gulp
MIT License
496 stars 88 forks source link

fix: the function is exported by default #113

Closed y805939188 closed 3 years ago

y805939188 commented 3 years ago

the code "export declare function replace(" in line 19 cannot export the replace function correctly in typescript. must use " import { replace } from 'gulp-replace' " to export, but at this time the "replace" is not a function.

lazd commented 3 years ago

@jon2180 can you review this PR?

lazd commented 3 years ago

@y805939188 are you saying that, without your patch, it works if you do import { replace } from 'gulp-replace';, or that your patch fixes that?

lazd commented 3 years ago

@y805939188 please elaborate on what was broken, what works now, and how you're importing the package after this update. Thanks!

manuth commented 3 years ago

Hello @lazd As far as I can see, this PR, too, contains incorrect type-declarations.

As I, of course, cannot edit PRs, I opened an own one: #114

manuth commented 3 years ago

As seen here, this module doesn't use the export default-behavior: https://github.com/lazd/gulp-replace/blob/14df43bda9934796befeca6dfabea6b93659bec2/index.js#L7

TypeScript's

export default replace;

Would be the equivalent of

module.exports.default = function replace() { }

or

export default function replace() { }

Therefore, changes made in this PR are incorrect.

jon2180 commented 3 years ago

@lazd @manuth Maybe it's because it's too late in the city where I lived. I am a little dizzy. And I made a re-check.

About this problem, there are official document of export= and official document of default exports

And I refered the type definition of jQuery

Following export method is suitable for CommonJS.

// ts export
export = replace;
// js import
const replace = require('gulp-replace');
// ts import
import zip = require("gulp-replace");

Instead, the export default replace is es6 version export

Result: I think PR #114 should be correct.

manuth commented 3 years ago

@jon2180 No worries - meanwhile where I live, it's evening 😂 And no, it is not.

As seen here: https://github.com/lazd/gulp-replace/blob/14df43bda9934796befeca6dfabea6b93659bec2/index.js#L7 gulp-replace neither uses the module.exports.default = function replace() { } nor the export default function replace() { }.

The type-declaration suitable for gulp-replace is the export =-syntax.

manuth commented 3 years ago

Take chalk, for example. It uses the export default-syntax as seen here: https://github.com/chalk/chalk/blob/f8a3642a8107f6029c6923b72a43c35a1065a336/source/index.js#L248 Therefore, the type-declaration suitable here is export default [...]: https://github.com/chalk/chalk/blob/f8a3642a8107f6029c6923b72a43c35a1065a336/index.d.ts#L339

On the other hand, yeoman-generator, for example, uses the module.exports =-approach: https://github.com/yeoman/generator/blob/357932222b7806e6d3e60c44d5cbdd9b2da74dcf/lib/index.js#L1415 The suitable type-declaration here is export =: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/28bcd40f99951d1c859a78d4d9edec3f721f70d2/types/yeoman-generator/index.d.ts#L902

jon2180 commented 3 years ago

@manuth Yeah. I have read the docs and tried in typescript playground with your help. Finally, I get the following result.

// js export
module.exports = function () {}
// to .d.ts
declare function _exports(): void;
export = _exports;

and

// js export
module.exports.default = function () {}
// to .d.ts
declare function _default(): void;
export default _default;

Is this right understanding?

I think you are correct. I really appreciate your comments for helping me understand.

manuth commented 3 years ago

Yeah - thanks for your patience. Honestly I had a lot of confusion about TypeScript's export default vs. export =-topic myself in past projects so I really can relate 😅

jon2180 commented 3 years ago

@lazd I am so sorry that i made a mistake which crashed your plugin. And I will learn more and be careful afterward. Your consideration will make me feel pretty grateful.

jon2180 commented 3 years ago

@manuth So wonderful.

Now, I have learned a lot about this too.

manuth commented 3 years ago

@jon2180 no worries, I'm glad I could help ✨