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

adapter-cloudflare@1.0.0-next.36 ambient.d.ts breaks DOM manipulation function types #8268

Closed
tv42 opened this issue Dec 27, 2022 · 3 comments · Fixed by #8483
Closed

adapter-cloudflare@1.0.0-next.36 ambient.d.ts breaks DOM manipulation function types #8268

tv42 opened this issue Dec 27, 2022 · 3 comments · Fixed by #8483

Comments

@tv42
Copy link

tv42 commented Dec 27, 2022

Describe the bug

(This bug also occurs with @sveltejs/adapter-cloudflare@1.0.0, I'm talking about 1.0.0-next.36 because that's what introduced the issue.)

I need a workaround for an ugly Safari bug, which forces me to do some HTML rewriting before I use @html. I use DOMParser.parseFromString and some light DOM manipulation for this. This function is only called when browser from $app/environment is true.

Upgrading to @sveltejs/adapter-cloudflare@1.0.0-next.36 broke the Typescript DOM API:

Error: Argument of type 'HTMLDivElement' is not assignable to parameter of type 'Content'. (ts)
      div.textContent = "\n";
      child.after(div);
    }

That child there is of type Element, and Element.after absolutely should take a HTMLDivElement. Looking up the type definition of child.after in an IDE leads to node_modules/@cloudflare/workers-types/index.d.ts which has the incorrect

declare type Content = string | ReadableStream | Response;
...
interface Element {
   ...
  after(content: Content, options?: ContentOptions): Element;

It seems PR #6917 caused ambient.d.ts referencing Cloudflare's types make them override the DOM API types:

/// <reference types="@cloudflare/workers-types" />

It seems this only triggers if the tsconfig workspace contains a file that actually imports @sveltejs/adapter-cloudflare. In my case, I have svelte.config.js added to tsconfig so I get useful IDE feedback.

Reproduction

https://github.com/tv42/sveltekit-issue-cloudflare-dom

The latest commit has svelte-check complaining

/blah/bug-dom-worker-2/src/routes/+page.svelte:9:17
Error: Argument of type 'HTMLSpanElement' is not assignable to parameter of type 'Content'. (ts)
      span.innerText = "dynamic content";
      h1.append(span);
	}

You can checkout the commit before it switched to @sveltejs/adapter-cloudflare@1.0.0-next.36, run npm i, and observe npm run check be happy.

Note how triggering this hinges on the commit that edited tsconfig.json.

Logs

No response

System Info

System:
    OS: Linux 6.0 NixOS 22.11 (Raccoon) 22.11 (Raccoon)
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
    Memory: 36.27 GB / 62.60 GB
    Container: Yes
    Shell: 5.1.16 - /run/current-system/sw/bin/bash
  Binaries:
    Node: 16.18.1 - /nix/store/gqbqpgmr7s8b3km14m54ys5kilx0akcc-nodejs-16.18.1/bin/node
    npm: 8.19.2 - /nix/store/gqbqpgmr7s8b3km14m54ys5kilx0akcc-nodejs-16.18.1/bin/npm
  Browsers:
    Firefox: 108.0.1
  npmPackages:
    @sveltejs/adapter-cloudflare: ^1.0.0-next.36 => 1.0.0-next.36
    @sveltejs/kit: ^1.0.0 => 1.0.1
    svelte: ^3.54.0 => 3.55.0
    vite: ^4.0.0 => 4.0.3

Severity

blocking an upgrade

Additional Information

No response

@Conduitry
Copy link
Member

Presumably you can use a @ts-ignore or @ts-expect-error comment as a workaround and avoiding this blocking an upgrade?

@tv42
Copy link
Author

tv42 commented Dec 27, 2022

Yes, if I litter enough @ts-expect-error in the source it stops complaining. I can make progress now, thank you.

(It still shouldn't complain, and now it's just a minefield of what web standard API does Cloudflare break next.)

@tv42
Copy link
Author

tv42 commented Dec 27, 2022

This also broke a place where I do

const result = (await response.json()) as MyType;

with eslint complaining

  58:18  error  This assertion is unnecessary since it does not change the type of the expression  @typescript-eslint/no-unnecessary-type-assertion

and that seems to be because Cloudflare overrides response.json to be

  json<T>(): Promise<T>;

while node_modules/typescript/lib/lib.dom.d.ts says

interface Body {
    readonly body: ReadableStream<Uint8Array> | null;
    readonly bodyUsed: boolean;
    arrayBuffer(): Promise<ArrayBuffer>;
    blob(): Promise<Blob>;
    formData(): Promise<FormData>;
    json(): Promise<any>;
    text(): Promise<string>;
}

This spooky action at a distance property is like the worst part of Typescript :-(

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

Successfully merging a pull request may close this issue.

3 participants