Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Types of property 'headers' are incompatible (with new 3.1.0) #95

Closed
Jauminha opened this issue Mar 18, 2021 · 17 comments · Fixed by #97
Closed

Types of property 'headers' are incompatible (with new 3.1.0) #95

Jauminha opened this issue Mar 18, 2021 · 17 comments · Fixed by #97

Comments

@Jauminha
Copy link

(First of all thanks for this amazing package!)

We are using cross-fetch to use the same client calls to our APIs from other APIs and any running application.
For our infrastructural needs, we created a small customFetch function on top of the fetch function so we could change the URL if a cloud parameter is passed or not:

export const customFetch = (input: RequestInfo, init: RequestInit, cloud?: CloudSubdomain | string): Promise<Response> => {
    let url = "";
    if (cloud) {
        url += createCloudUrl(cloud);
    }
    url += input;
    return fetch(url, init);
};

After updating to version 3.1.0 our code started to throw the following error:

Type 'Promise<import("/home/user/development/node_modules/cross-fetch/lib.fetch").Response>' is not assignable to type 'Promise<Response>'.
  Type 'import("/home/user/development/node_modules/cross-fetch/lib.fetch").Response' is not assignable to type 'Response'.
    Types of property 'headers' are incompatible.
      Type 'Headers' is missing the following properties from type 'Headers': [Symbol.iterator], entries, keys, values

22     return fetch(url, init);

I inspected the types from both node_modules/typescript/lib/lib.dom.d.ts and node_modules/cross-fetch/lib.fetch.d.ts and I see no difference apart from this:

declare var Headers: {
    prototype: Headers;
    new(init?: HeadersInit): Headers;
};

Hope you can shed some light on this.
Thanks for your time!

@jstewmon
Copy link
Contributor

The problem is that we're missing an augmentation from lib.dom.iterable.d.ts:

interface Headers {
    [Symbol.iterator](): IterableIterator<[string, string]>;
    /**
     * Returns an iterator allowing to go through all key/value pairs contained in this object.
     */
    entries(): IterableIterator<[string, string]>;
    /**
     * Returns an iterator allowing to go through all keys of the key/value pairs contained in this object.
     */
    keys(): IterableIterator<string>;
    /**
     * Returns an iterator allowing to go through all values of the key/value pairs contained in this object.
     */
    values(): IterableIterator<string>;
}

I think I can fix this by adding a graft for that specific interface and use declaration merging to get the right type definition in the cross-fetch index.d.ts.

@Jauminha
Copy link
Author

Awesome! Waiting for the merge 👍

@lquixada
Copy link
Owner

Fix released on 3.1.2. 👍 thanks everyone!

@ThomWright
Copy link

@lquixada This is still happening for me on 3.1.2.

@jstewmon
Copy link
Contributor

🤦 Sorry again - I was trying to quickly think of a way to fix this problem and I failed to really internalize the problem before proposing a solution.

I've got a new approach that I think will really work. I'll open a new PR shortly, including how I've tested it against the code sample in this issue.

jstewmon added a commit to jstewmon/cross-fetch that referenced this issue Mar 22, 2021
lib.fetch.d.ts includes all type definitions from the dom and
dom.iterable libs which were reachable from the original cross-fetch
index.d.ts file:

```
git checkout 7b19cda -- index.d.ts
```

lib.fetch.d.ts was generated with the following command:

```
cat <<EOF > .ts-graftrc.yaml \
&& npx ts-graft@2.0.0-1 \
&& rm .ts-graftrc.yaml \
grafts:
  - source: index.d.ts
    output: lib.fetch.d.ts
    include:
      - dom
      - dom.iterable
EOF
```

index.d.ts was then updated to its new state and verified with the
following command:

```
npx tsc --lib ES6 index.d.ts
```

Issue lquixada#95 was validated as follows:

```
cat <<EOF > test.ts \
&& npx tsc --target ES6 --moduleResolution node --noEmit  test.ts \
&& rm test.ts
import fetch from "./";
export const customFetch = (
  input: RequestInfo,
  init: RequestInit,
): Promise<Response> => {
  let url = "";
  url += input;
  return fetch(url, init);
};
EOF
```

n.b. consumers must include the dom.iterable lib, which is implied by
the ES6 target, lest their globals lack the required members defined
by dom.iterable.
jstewmon added a commit to jstewmon/cross-fetch that referenced this issue Mar 22, 2021
lib.fetch.d.ts includes all type definitions from the dom and
dom.iterable libs which were reachable from the original cross-fetch
index.d.ts file:

```
git checkout 7b19cda -- index.d.ts
```

lib.fetch.d.ts was generated with the following command:

```
cat <<EOF > .ts-graftrc.yaml \
&& npx ts-graft@2.0.0-1 \
&& rm .ts-graftrc.yaml \
grafts:
  - source: index.d.ts
    output: lib.fetch.d.ts
    include:
      - dom
      - dom.iterable
EOF
```

index.d.ts was then updated to its new state and verified with the
following command:

```
npx tsc --lib ES6 index.d.ts
```

Issue lquixada#95 was validated as follows:

```
cat <<EOF > test.ts \
&& npx tsc --target ES6 --moduleResolution node --noEmit  test.ts \
&& rm test.ts
import fetch from "./";
export const customFetch = (
  input: RequestInfo,
  init: RequestInit,
): Promise<Response> => {
  let url = "";
  url += input;
  return fetch(url, init);
};
EOF
```

n.b. consumers must include the dom.iterable lib, which is implied by
the ES6 target, lest their globals lack the required members defined
by dom.iterable.
@Jauminha
Copy link
Author

I guess the issue should be reopened until the fix has been merged? More people will stumble upon this

lquixada pushed a commit that referenced this issue Mar 27, 2021
lib.fetch.d.ts includes all type definitions from the dom and
dom.iterable libs which were reachable from the original cross-fetch
index.d.ts file:

```
git checkout 7b19cda -- index.d.ts
```

lib.fetch.d.ts was generated with the following command:

```
cat <<EOF > .ts-graftrc.yaml \
&& npx ts-graft@2.0.0-1 \
&& rm .ts-graftrc.yaml \
grafts:
  - source: index.d.ts
    output: lib.fetch.d.ts
    include:
      - dom
      - dom.iterable
EOF
```

index.d.ts was then updated to its new state and verified with the
following command:

```
npx tsc --lib ES6 index.d.ts
```

Issue #95 was validated as follows:

```
cat <<EOF > test.ts \
&& npx tsc --target ES6 --moduleResolution node --noEmit  test.ts \
&& rm test.ts
import fetch from "./";
export const customFetch = (
  input: RequestInfo,
  init: RequestInit,
): Promise<Response> => {
  let url = "";
  url += input;
  return fetch(url, init);
};
EOF
```

n.b. consumers must include the dom.iterable lib, which is implied by
the ES6 target, lest their globals lack the required members defined
by dom.iterable.
@lquixada
Copy link
Owner

@Jauminha yeah, makes sense. I'm still working on this though!

@lquixada lquixada reopened this Mar 27, 2021
@lquixada
Copy link
Owner

lquixada commented Mar 28, 2021

@Jauminha just released an alpha version. can you try your code with npm install cross-fetch@3.1.3-alpha.4

@iCrawl
Copy link

iCrawl commented Mar 28, 2021

This does not work at all now, because fetch is now a type only and cannot be used as a value/function anymore.

@lquixada
Copy link
Owner

@iCrawl hi, can you try again with npm install cross-fetch@3.1.3-alpha.6?

@Jauminha
Copy link
Author

Sorry, email notifications went into spam.
Alpha.4 same as iCrawl.
Alpha.6 👍, works for me.

@jstewmon
Copy link
Contributor

@lquixada was there a problem with the index.d.ts from #100 you were trying to fix?

The changes you made put us nearly full circle where importing cross-fetch pollutes the globals with all the types that are in lib.fetch.d.ts. The folks using dom lib don't notice any problem b/c they already have all those types in their globals.

But, server app developers get incorrect type checking feedback. For example, this should not compile without using the dom lib, but it does because the compiler finds a global type definition for Request:

import fetch from "cross-fetch";

const req = new Request("http://example.com");

image

@lquixada
Copy link
Owner

@jstewmon I feel the issue we're trying to address here is the conflict reported by @Jauminha regarding Headers (and any other fetch API member). So far cross-fetch@3.1.3-alpha.6 seems to fix the problem.

Regarding the code you've mentioned, not sure why it should not compile. In Typescript, global types are a valid way to provide typing.

import fetch from "cross-fetch";

const req = new Request("http://example.com");

The new cross-fetch version provides types from both imports and globals. The reason for this is that it can be used as a ponyfill (import fetch from "cross-fetch") or as a polyfill (import "cross-fetch/poyfill";). PR #100 doesn't address the latter case.

That being said, I'm not saying that the current approach (3.1.3-alpha.6) should be the final one. We could certainly go back to discuss it but probably in another github issue or PR.

@jstewmon
Copy link
Contributor

Regarding the code you've mentioned, not sure why it should not compile. In Typescript, global types are a valid way to provide typing.

In the example, I used Request as a class (concrete type), not an interface (abstract type), so the code will fail at runtime on the server. To be correct, it must be written as:

import fetch, { Request } from "cross-fetch";

const req = new Request("https://example.com");

The reason for this is that it can be used as a ponyfill (import fetch from "cross-fetch") or as a polyfill (import "cross-fetch/poyfill";). PR #100 doesn't address the latter case.

This requires separate type definition files. The global vars and types should be declared in polyfill.d.ts, which only needs the concrete declarations which are globally polyfilled and their supporting type declarations (no imports / exports are needed).

@lquixada
Copy link
Owner

@jstewmon interesting approach! Do you feel we should open a new issue for that?

@jstewmon
Copy link
Contributor

I would say this has been a rather tricky issue to sort out, and this discussion has revealed much of the nuance in addressing it properly, so I lean towards keeping the details consolidated here. But, I don't think the choice has much consequence either way, so if you prefer a new issue, that's fine too.

@lquixada
Copy link
Owner

thanks all! 3.1.3 has just been released 👍

@jstewmon yeah I agree it's a tricky issue. I've released a new version so everyone gets the compilation error solved. Let's open a new issue/PR to address the global pollution and the polyfill case! Closing this one for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants