preact icon indicating copy to clipboard operation
preact copied to clipboard

New typings broke custom elements for me

Open surma opened this issue 7 years ago • 52 comments

The new typings introduced in #1116 broke custom elements for me. Is there a workaround, am I using JSX.IntrinsicElements wrong or do we need a fix for preact’s typings?

Test case (attach this to test/ts/VNode-test.s):

class TestCE extends HTMLElement {}

declare global {
  namespace JSX {
    interface IntrinsicElements {
      ["test-ce"]: Partial<TestCE>;
    }
  }
}

const MyTestCustomElement = <test-ce><div>hai</div></test-ce>;

Error:

test/ts/VNode-test.tsx:78:30 - error TS2322: Type '{ children: Element; }' is not assignable to type 'Partial<TestCE>'.
  Types of property 'children' are incompatible.
    Type 'Element' is not assignable to type 'HTMLCollection | undefined'.
      Type 'Element' is not assignable to type 'HTMLCollection'.
        Property 'namedItem' is missing in type 'Element'.

const MyTestCustomElement = <test-ce><div>hai</div></test-ce>;

cc @marvinhagemeister

surma avatar Aug 09 '18 22:08 surma

That's a tough one to crack. Somehow the children property is incorrectly inferred in this case. As a workaround you can overwrite the type of children:

declare global {
  namespace JSX {
    interface IntrinsicElements {
      // Overwrite children prop
      ["test-ce"]: Partial<TestCE & { children: any}>;
    }
  }
}

FYI: Simpler test case, because the TypeScript tests are not executed inside a browser:

declare global {
  namespace JSX {
    interface IntrinsicElements {
      ["test-ce"]: Partial<HTMLElement>;
    }
  }
}

const MyTestCustomElement = <test-ce><div>hai</div></test-ce>;

marvinhagemeister avatar Aug 10 '18 05:08 marvinhagemeister

@marvinhagemeister Thanks for the workaround! Yeah, no idea what to do about this one.

surma avatar Aug 10 '18 09:08 surma

... I just realized that HTMLElement defines children as HTMLCollection.

surma avatar Aug 10 '18 11:08 surma

@surma ohh good catch, didn't know about that one 👍

marvinhagemeister avatar Aug 10 '18 11:08 marvinhagemeister

In this case, is HTMLElement the same as JSX.HTMLElement?

developit avatar Aug 10 '18 15:08 developit

I think this is an unfortunate clash of names between what JSX considers as children and the children property of HTML elements

If you wanted to replace the type of children for the definition in IntrinsicElements rather than just widen it you could define

type PreactCustomElement<T, Children = preact.ComponentChildren> = {
	[P in keyof T]?: P extends "children" ? Children : T[P]
}

It defaults (ie when using as PreactCustomElement<TestCE>) to allowing anything but it can accept only string children for example by doing PreactCustomElement<TestCE, string>

This is still less than ideal since it will let you do things like pass addEventListener as a property, I'll think about a better solution for that

Alexendoo avatar Aug 10 '18 18:08 Alexendoo

Okay so I think I've wrapped my head around it a bit now. I believe that as far as IntrinsicElements is concerned it actually shouldn't reference a custom element class, nor a HTML class definition.

The properties defined in an IntrinsicElements entry for a custom element are the attributes it accepts, since it's like a regular HTML element. As such they are not surfaced anywhere in the type definition of TestCE—the way to get/set them is by using the DOM APIs that return just strings, for example getAttribute() and dataset.

I would say the best way to type it, then, is to treat it the same way the other intrinsic elements are

declare global {
  namespace JSX {
    interface IntrinsicElements {
      ["test-ce"]: HTMLAttributes;
    }
  }
}

This will give you the same behaviour as say a regular div: it will accept the common attributes and data-*. If your element accepts extra attributes then you can use an explicit type union such as

declare global {
  namespace JSX {
    interface IntrinsicElements {
      ["color-picker"]: HTMLAttributes & {
        // Required attribute
        space: "rgb" | "hsl" | "hsv",
        // Optional attribute
        alpha?: boolean
      };
    }
  }
}

Alexendoo avatar Aug 10 '18 19:08 Alexendoo

@Alexendoo Awesome, thanks for taking up the torch and getting to the bottom of it. That thought about attributes crossed my mind but somehow I didn't pursue it further. Reading your comment now it makes so much more sense to go that way. Kudos to you and thanks for the great explanation. That's cleared a few things up for me 👍

@surma If you feel like it isn't feel free to ping me to reopen this issue.

EDIT: Perhaps we should add a note about it to our docs/Readme? cc @developit

marvinhagemeister avatar Aug 10 '18 19:08 marvinhagemeister

Definitely worth adding a note. Perhaps a page on the Preact site about usage with TypeScript? Or a .md here.

developit avatar Aug 12 '18 17:08 developit

@marvinhagemeister

Did such a note get added? I still can't figure out how to do this as of Preact X rc0.

https://github.com/kuhe/cannot-extend-instrinsic-elements-demo

kuhe avatar Jul 19 '19 18:07 kuhe

Thanks for pinging me. I'll reopen this ticket and mark it as something for the docs. I'm working on the new ones for X and we should write a section about this in the next weeks.

marvinhagemeister avatar Jul 19 '19 19:07 marvinhagemeister

For future reference, extending multi-namespaced aspects is weird (see #1714 ). For @kuhe: Easiest to do:

// global.d.ts
declare namespace preact.JSX {
  interface IntrinsicElements {
    [tag: string]: any;
  }
}

pmkroeker avatar Jul 19 '19 21:07 pmkroeker

@pmkroeker

I put that into a global.d.ts file and it still doesn't compile. I've tried a number of variations of that declaration to get it to merge but have not found a working form.

My setup, now including global.d.ts, is in https://github.com/kuhe/cannot-extend-instrinsic-elements-demo.

kuhe avatar Jul 22 '19 14:07 kuhe

@kuhe You have to make sure to include the file in your tsconfig.json file.

{
  "compilerOptions": ...
  "include": [
    "src",
    "global.d.ts"
  ]
}

Something like the above.

pmkroeker avatar Jul 22 '19 15:07 pmkroeker

I tried this include array and it doesn't make a difference.

I know the global.d.ts file was being included even without this include[] because the TS compiler errors would change depending on what I had written in it.

Nevertheless, I updated my demo repo to include include[] in tsconfig.json, and an additional interface in global.d.ts that is used in main.tsx.

kuhe avatar Jul 22 '19 16:07 kuhe

Strange, it seems as though the behaviour has changed since I last looked into this. Investigating further!

pmkroeker avatar Jul 22 '19 16:07 pmkroeker

Your jsx pragma setting can affect it also. I think using preserve on your root tsconfig and then assigning your desired pragma in your build script is the way to go

image

jeremy-coleman avatar Jul 22 '19 21:07 jeremy-coleman

@jeremy-coleman

~Using preserve leaves out type-checking on JSX, which is something we need to ... preserve.~

I copied your screenshot's file contents and structure.

Using preserve without anything in global.d.ts still has a "strict" violation

JSX element implicitly has type 'any' because no interface 'JSX.IntrinsicElements' exists.

Using preserve with IntrinsicElement declarations has the same type errors as before

node_modules/preact/src/jsx.d.ts:17:35 - error TS2694: Namespace 'preact' has no exported member 'VNode'.

kuhe avatar Jul 23 '19 19:07 kuhe

Thanks, Peter.

I will work around it with a slight local modification of Preact X for my team's project(s) so that we can upgrade from 8.x.

kuhe avatar Jul 23 '19 21:07 kuhe

To clarify: Is there currently a workaround for using CEs that does not involve monkey-patching Preact X?

surma avatar Jul 28 '19 23:07 surma

A definate workaround is to use h instead of jsx

jeremy-coleman avatar Jul 29 '19 13:07 jeremy-coleman

As per discussion with @surma, this likely circumvents the issue (but is not a solution):

const TestCe = 'test-ce';
const jsx = <TestCe><div>hai</div></TestCe>

(JSX turns any tagname with a leading uppercase character or dotted identifier into a variable reference)

developit avatar Jul 29 '19 16:07 developit

Sadly, TypeScript seems too smart there, too 😅

const PinchZoom = "pinch-zoom"

export default class Inspector extends Component {
  render() {
    return (
      <PinchZoom/>
    );
  }
}

Errors with

src/components/inspector/index.tsx:15:7 - error TS2339: Property 'pinch-zoom' does not exist on type 'JSX.IntrinsicElements'.

15       <PinchZoom />
         ~~~~~~~~~~~~~

src/components/inspector/index.tsx:15:8 - error TS2604: JSX element type 'PinchZoom' does not have any construct or call signatures.

15       <PinchZoom />
          ~~~~~~~~~

surma avatar Sep 09 '19 22:09 surma

Actually, with

const PinchZoom = "pinch-zoom" as any;

it does compile. Pretty bad, but I’ll take it.

surma avatar Sep 09 '19 22:09 surma

@surma I think I found a solution. Can you verify that this works?

import { h } from "preact";
// We just import the raw jsx types again. TS will treat it as a
// new namespace, which can be used to avoid a circular type
// reference due to module augmentation below
import { JSXInternal as JSXI } from "preact/src/jsx";

class TestCE extends HTMLElement {}

declare module "preact" {
	namespace JSXInternal {
		// We need to extend the previous interface, otherwise we'll
		// loose the previous interface members completely ¯\_(ツ)_/¯
		interface IntrinsicElements extends JSXI.IntrinsicElements {
			["test-ce"]: Partial<TestCE>;
		}
	}
}

const MyTestCustomElement = (
	<test-ce>
		<div>hai</div>
	</test-ce>
);

marvinhagemeister avatar Sep 10 '19 17:09 marvinhagemeister

@marvinhagemeister it works for elements, however

import { h } from 'preact'

interface IProps {
  onClick?: h.JSX.MouseEventHandler
  //              ^^^^^^^^^^^^^^^^^ it's broken here since then
}

Though solvable by workrounds like import { JSXInternal } from 'preact', I don't think having to do that in most components makes the solution resonable.

somarlyonks avatar Sep 30 '19 03:09 somarlyonks

@marvinhagemeister I think this might be the proper way to do it

import preact from 'preact'
import { JSXInternal } from 'preact/src/jsx'

declare module 'preact/src/jsx' {
    namespace JSXInternal {
	interface IntrinsicElements {
	    ["test-ce"]: Partial<TestCE>
	}
    }
}

It merges namespace JSXInternal in this way.

somarlyonks avatar Sep 30 '19 03:09 somarlyonks

@talentlessguy that's not an issue with custom elements, Header would be a regular component

ForsakenHarmony avatar Nov 24 '19 23:11 ForsakenHarmony

@talentlessguy that's not an issue with custom elements, Header would be a regular component

so it is a problem with Emotion?

v1rtl avatar Nov 25 '19 09:11 v1rtl