lquixada / cross-fetch

Universal WHATWG Fetch API for Node, Browsers and React Native.
MIT License
1.67k stars 104 forks source link

TypeScript import incorrectly makes fetch global #59

Closed andrewrothman closed 4 years ago

andrewrothman commented 4 years ago

Hi there,

I've been using cross-fetch in a Node.js TypeScript project. When used in "non-global" form, I am seeing incorrect successful TypeScript compilation results which cause unexpected "ReferenceError" throws at runtime.

Example Project Setup:

mkdir test-project
cd test-project
yarn init
yarn add typescript cross-fetch
# specifying lib so that 'dom' isnt imported, as this is a Node project
npx tsc --init --lib es6
echo "import fetch from \"cross-fetch\";" > one.ts
echo "fetch(\"\", {});" > two.ts

Issue:

TypeScript compiles the files correctly, as the "index.d.ts" file bundled with cross-fetch exposes the "fetch" global to both "one.ts" and "two.ts" even though it is only imported in "one.ts". I think this is because the "dom" lib is referenced here.

If I try to compile and run "two.js", I get "ReferenceError: fetch is not defined" as is expected.

npx tsc

node two.js

/Users/andrew/test-project/other-file.js:2
fetch("", {});
^

ReferenceError: fetch is not defined
    at Object.<anonymous> (/Users/andrew/test-project/other-file.js:2:1)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:283:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)

The correct result would be for TypeScript compilation to fail, as "fetch" should not become a project-wide global, and instead only be available in the file it is imported in.

Thanks so much for your work on cross-fetch,

Andrew

beenotung commented 4 years ago

Is there anyway to undo the /// reference? In the similar fashion as tslint:disable and tslint:enable pair

lquixada commented 4 years ago

Hey @andrewrothmanm, thanks for this very detailed issue! It seems the issue is indeed the reference directive in cross-fetch's index.d.ts. Would you have any suggestions for a workaround?

lquixada commented 4 years ago

@andrewrothman after some thought, I feel the right approach for you would be migrating to node-fetch as you seemingly doesn't need it running in a browser and making fetch non-global would be cumbersome. Closing this issue.

bisubus commented 4 years ago

I'm having the same problem and was about to open an issue. This behaviour is incorrect because Node entry point is a ponyfill, there's no global fetch. Also it pollutes global namespace with irrelevant DOM types.

Also, the difference between cross-fetch and node-fetch is that this package uses a fix for relative protocol. Is there a reason why this fix it wasn't contributed to node-fetch?