Removed node-fetch package and used native fetch

[theothirteen]

Fixes #589 Issue. Option 4 Suboption 1.

• Tests pass
• Appropriate changes to README are included in the PR
• Types updated

Changes

• Remove node-fetch dependency
• Removed DOM lib from tsconfig
• Added @types/node-fetch as dev dependencies to get the updated types for fetch

[antongolub]

I'd suggest smth like "engines": { "node": ">= 18.0.0" } instead of all these node version checks and err notices.

[theothirteen]

@antongolub Thank you for the review. Please correct me If I am wrong. Once I upgrade the node engine field then test cases will work without any node version check but in the case of the Fetch function if a user tries to run the zx with node 16 then we have to print the error(#589 (comment)) message that fetch is not supported so don't we need a check there?

[antonmedv]

About "engines": { "node": ">= 18.0.0" }, this will prevent installing zx on node16. What if somebody not using fetch? Just $?

[midzelis]

instead of checking version, why not just check if fetch exists?

if ('fetch' in globalThis) or if (globalThis.fetch)

[theothirteen]

@midzelis Thank you for the feedback. Updated that condition. Please take a look and let me know if you have any doubts.

[njfamirm]

Thank you @theothirteen !
Considering that node native fetch doesn't need any package to work, even in types, you can generally remove @types/node-fetch and use nodejs types,

I'm do this in this branch and create pr in your repo: theothirteen#1
You can include my changes in this pr by merging that!

Chnges:

• add DOM as lib into tsconfig
• fully remove node-fetch type

[theothirteen]

Thank you @theothirteen ! Considering that node native fetch doesn't need any package to work, even in types, you can generally remove @types/node-fetch and use nodejs types,

I'm do this in this branch and create pr in your repo: theothirteen#1 You can include my changes in this pr by merging that!

Chnges:

• add DOM as lib into tsconfig
• fully remove node-fetch type

Hello @njfamirm Thank you for the PR and suggestions.

The reason why I have added @types/node-fetch is to use types. Currently RequestInfo and RequestInit types are not available in @types/node so just for the types and for a better development experience keep the types. Also as it's a dev dependency so does not affect bundle size. Please let me know if you have alternate solutions for that issue.

Furthermore, as node-fetch is dependent on the DOM lib and currently we are not using it so removed it.

[njfamirm]

Thanks for your review @theothirteen

The reason why I have added @types/node-fetch is to use types. Currently RequestInfo and RequestInit types are not available in @types/node so just for the types and for a better development experience keep the types. Also as it's a dev dependency so does not affect bundle size. Please let me know if you have alternate solutions for that issue.

but RequestInfo & RequestInit is built-in in typescript. (you can see this with vscode autocomplete and following type definition)

Also if this types not exists, we have build error, but current have no error.
I think it is a mistake to use node-fetch types for native fetch.

Furthermore, as node-fetch is dependent on the DOM lib and currently we are not using it so removed it.

if we want to use fetch on typescript, we must add DOM lib.

[njfamirm]

I'm update my pr and resolve conflicts, so ready for rereview

[theothirteen]

Thanks for your review @theothirteen

The reason why I have added @types/node-fetch is to use types. Currently RequestInfo and RequestInit types are not available in @types/node so just for the types and for a better development experience keep the types. Also as it's a dev dependency so does not affect bundle size. Please let me know if you have alternate solutions for that issue.

but RequestInfo & RequestInit is built-in in typescript. (you can see this with vscode autocomplete and following type definition)

Also if this types not exists, we have build error, but current have no error. I think it is a mistake to use node-fetch types for native fetch.

Furthermore, as node-fetch is dependent on the DOM lib and currently we are not using it so removed it.

if we want to use fetch on typescript, we must add DOM lib.

@njfamirm Totally agree with you but in the initial requirement, it is mentioned that we can drop the DOM lib if we go with the native fetch. That is the reason I have removed the DOM lib. @antonmedv Could you please provide your inputs if we want to keep the DOM lib or not?

[njfamirm]

@njfamirm Totally agree with you but in the initial requirement, it is mentioned that we can drop the DOM lib if we go with the native fetch. That is the reason I have removed the DOM lib. @antonmedv Could you please provide your inputs if we want to keep the DOM lib or not?

@theothirteen Yes you are right 👍🏻
But the problem is that fetch actually needs DOM in typescript, but we solve this problem by using any type, which of course cannot be called solving! just ignoring 🫠

[antonmedv]

I'd like to drop DOM

[njfamirm]

Whats up?

[antongolub]

Resolved in #722