lquixada / cross-fetch

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

Compilation error with version 3.1.2 #101

Closed sergioness closed 3 years ago

sergioness commented 3 years ago

First of all, thanks for your powerful package!

Problem

Upgrading from 3.1.1 to 3.1.2 causes the following errors:

node_modules/cross-fetch/index.d.ts(1,13): error TS1005: '=' expected.
node_modules/cross-fetch/index.d.ts(10,1): error TS1109: Expression expected.
node_modules/cross-fetch/index.d.ts(10,8): error TS1005: ';' expected.
node_modules/cross-fetch/index.d.ts(11,13): error TS1005: '=' expected.
node_modules/cross-fetch/index.d.ts(11,45): error TS1005: ';' expected.

Environment

"cross-fetch": "3.1.2",
"typescript": "3.6.5",
"rollup": "1.32.1",
"rollup-plugin-typescript2": "0.20.1"

Workaround

Downgrading to version 3.1.1 (or "cross-fetch": "^3.1 <=3.1.1 || ^3.1 >3.1.2").

lquixada commented 3 years ago

@sergioness hey, thanks for reporting this issue. Can you run npm install cross-fetch@3.1.3-alpha.6 and check if the problem persists?

sergioness commented 3 years ago

@sergioness hey, thanks for reporting this issue. Can you run npm install cross-fetch@3.1.3-alpha.6 and check if the problem persists?

@lquixada, thanks, it compiles fine with cross-fetch@3.1.3-alpha.6.

I think the issue is resolved now.

lquixada commented 3 years ago

@sergioness thanks! just released 3.1.3 👍

sergioness commented 3 years ago

hey, @lquixada, it looks like some typings still missing there. Not sure how it did compile that time with no errors, probably my bad. Sorry for misinforming in my last comment 😓

Here are the logs now on tsc with both cross-fetch@3.1.3-alpha.6 and cross-fetch@3.1.3:

aaa: error TS2304: Cannot find name 'HTMLVideoElement'.
...
bbb: error TS2304: Cannot find name 'window'.
bbb: error TS2304: Cannot find name 'requestAnimationFrame'.
bbb: error TS2584: Cannot find name 'document'. Do you need to change your target library? Try changing the `lib` compiler option to include 'dom'.
...
ccc: error TS2304: Cannot find name 'addEventListener'.
ccc: error TS2304: Cannot find name 'removeEventListener'.
....
ddd: error TS2584: Cannot find name 'document'. Do you need to change your target library? Try changing the `lib` compiler option to include 'dom'.

a little more details on my env:

package.json

"@types/jasmine": "3.6.9",
"@types/node": "11.15.50",
"cross-fetch": "3.1.3",

tsconfig.json

{
  "compilerOptions": {
    "target": "ES5",
    "declaration": true,             
    "declarationDir": "./build",
    "declarationMap": true,               
    "sourceMap": true,                      
    "outDir": "./build",                  
    "rootDir": "./",
    "strict": false,                      
    "noImplicitAny": false,               
    "strictNullChecks": false,          
    "strictFunctionTypes": false,
    "strictBindCallApply": true,          
    "strictPropertyInitialization": false, 
    "noImplicitThis": true,             
    "alwaysStrict": true,
    "noUnusedLocals": false,                
    "noUnusedParameters": false,        
    "noImplicitReturns": true,              
    "noFallthroughCasesInSwitch": true, 
    "resolveJsonModule": true,
    "esModuleInterop": true,
  },
  "include": [
    "src/",
    "test/"
  ],
  "exclude": [
    "build/",
    "node_modules/",
  ] 
}

Notes:

  1. "lib": ["es5", "dom"] are not included, believing TypeScript includes them by itself matching the "target".
  2. No, there is no explicit node-fetch, nor @types/node-fetch dependencies.
ceisele-r commented 3 years ago

Yeah, we also out of a sudden after updating to 3.1.3 receive the errors @sergioness is reporting. It seems that after the update, the dom globals (window, localStorage etc.) are suddenly broken for TS.

lquixada commented 3 years ago

@sergioness I can see your target is set to "ES5". Typescript doesn't include the "dom" lib on ES5 (check here). The curious thing is that it does include it if you set it to "ES6" (check here).

So if you need the dom type declarations, I'd suggest to either add "dom" to the "lib" option in your tsconfig.json or change the target to "es6". cross-fetch should only account for the fetch types.

@ceisele-r please check if that's your case as well.

ceisele-r commented 3 years ago

@lquixada in our case, this is the tsconfig: image So dom was already included in the libs as we need the dom lib. Before the update, everything was working well, after updating to 3.1.3, I receive the compile breaks as mentioned.

lquixada commented 3 years ago

hey all! sorry for the trouble! adding local types to cross-fetch has been trickier than originally thought so I'm reverting those changes so everyone can run it safely. Please let me know if 3.1.4-alpha.0 works for you.

sergioness commented 3 years ago

Hi, @lquixada! Thanks for the really quick actions on this one!

With 3.1.4-alpha.0, it compiles with no errors (double checked this time 😅 )

sergioness commented 3 years ago

upd:

I guess I sorted the 1st part out:

Angular docs (actually everyone else too) pretty clear on these points.

original comment:

[part 1] Hmm, yes, indeed. I couldn't find any version of TS (briefly browsed from 2.0.2 through my 3.6.5 to latest one) where it does include those libs targeting ES5 though. I'm not even sure then on why people (these and those) so confidently saying about some defaults tsc includes for ES5 😐

@sergioness I can see your target is set to "ES5". Typescript doesn't include the "dom" lib on ES5 (check here). The curious thing is that it does include it if you set it to "ES6" (check here).

[part 2] Meanwhile, I noticed the original issue (on 3.1.2) came from import type feature introduced since TS v3.8, whereas my env is stick to TS v3.6. Using plain import (or upgrading TS to 3.8) works but that's probably not the case for you. Will import + importsNotUsedAsValues: "remove" similarly work for you?

ceisele-r commented 3 years ago

Hi @lquixada , I can confirm that with 3.1.4-alpha.0 it compiles without errors.

mshwery commented 3 years ago

This revert undoes #70, we should reopen that.