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

New typings broke custom elements for me #1180

Closed
surma opened this issue Aug 9, 2018 · 53 comments · Fixed by #2581
Closed

New typings broke custom elements for me #1180

surma opened this issue Aug 9, 2018 · 53 comments · Fixed by #2581

Comments

@surma
Copy link

surma commented Aug 9, 2018

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

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Aug 10, 2018

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>;

@surma
Copy link
Author

surma commented Aug 10, 2018

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

@surma
Copy link
Author

surma commented Aug 10, 2018

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

@marvinhagemeister
Copy link
Member

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

@developit
Copy link
Member

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

@Alexendoo
Copy link
Contributor

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
Copy link
Contributor

Alexendoo commented Aug 10, 2018

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
      };
    }
  }
}

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Aug 10, 2018

@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

@developit
Copy link
Member

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

@kuhe
Copy link

kuhe commented Jul 19, 2019

@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

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Jul 19, 2019

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.

@pmkroeker
Copy link
Contributor

pmkroeker commented Jul 19, 2019

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;
  }
}

@kuhe
Copy link

kuhe commented Jul 22, 2019

@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.

@pmkroeker
Copy link
Contributor

pmkroeker commented Jul 22, 2019

@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.

@kuhe
Copy link

kuhe commented Jul 22, 2019

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.

@pmkroeker
Copy link
Contributor

pmkroeker commented Jul 22, 2019

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

@jeremy-coleman
Copy link
Contributor

jeremy-coleman commented Jul 22, 2019

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

@kuhe
Copy link

kuhe commented Jul 23, 2019

@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'.

@pmkroeker
Copy link
Contributor

@kuhe
Copy link

kuhe commented Jul 23, 2019

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.

@surma
Copy link
Author

surma commented Jul 28, 2019

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

@jeremy-coleman
Copy link
Contributor

A definate workaround is to use h instead of jsx

@developit
Copy link
Member

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)

@JoviDeCroock

This comment has been minimized.

@talentlessguy

This comment has been minimized.

@talentlessguy

This comment has been minimized.

@hbroer
Copy link

hbroer commented Mar 6, 2020

Hi, I had a similar problem today. I have a npm package with custom elements which is owned by me, and also a project which depends on that package. My solution was to declare only a interface inside of the web-components package:

ui-title.ts

export class UiTitle { /* ... */ }

export interface UiTitleAttributes {
    title?: string;
}

index.ts

import {UiTitle, UiTitleAttributes} from "./components/ui-title";

export const defineElements = () => {
    customElements.define("ui-title", UiTitle);
};

export declare interface UiIntrinsicElements<T> {
    "ui-title": UiTitleAttributes & T;
}

this compiles to index.d.ts:

import { UiTitleAttributes } from "./components/ui-title";
export declare const defineElements: () => void;
export declare interface UiIntrinsicElements<T> {
    "ui-title": UiTitleAttributes & T;
}

Next I added src/types/jsx.d.ts to the dependent project:

import {JSXInternal as PreactJSX} from "preact/src/jsx";
import {UiIntrinsicElements} from "@gira/web-components";

declare module "preact" {
    namespace JSXInternal {
        interface IntrinsicElements extends PreactJSX.IntrinsicElements, UiIntrinsicElements<PreactJSX.HTMLAttributes> {}
    }
}

Now I can use the <ui-title/> element without an error.

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 5, 2020

Edited: Better solution

import { JSXInternal } from 'preact/src/jsx';
import { WhateverElementEvent, WhateverElement } from './custom-whatever';

declare module 'preact/src/jsx' {
  namespace JSXInternal {
    interface IntrinsicElements {
      'custom-whatever': WhateveElAttributes;
    }
  }
}

interface WhateveElAttributes extends JSXInternal.HTMLAttributes {
  someattribute?: string;
  onsomeevent?: (this: WhateverElement, ev: WhateverElementEvent) => void;
}

@hbroer
Copy link

hbroer commented Jun 11, 2020

It seems that the way we extend the JSXInternal namespace it drops the other types and interfaces in that namespace.

As an example the createContext function needs this to work again:

declare module "preact" {
    namespace JSXInternal {
        /* custom IntrinsicElements extension */
        interface ElementChildrenAttribute extends PreactJSX.ElementChildrenAttribute {}
    }
}

Some of the types inside of preacts index.d.ts file (181: createElement and 199: h) are loosing their types too and need the following interfaces to get added

declare module "preact" {
    namespace JSXInternal {
        /* custom IntrinsicElements extension */
        interface ElementChildrenAttribute extends PreactJSX.ElementChildrenAttribute {}
        interface HTMLAttributes extends PreactJSX.HTMLAttributes {}
        interface SVGAttributes extends PreactJSX.SVGAttributes {}
    }
}

And I wouldn't wonder if there are not more types which will be needed in other coding contexts.

I don't think it is a good idea to add all types and interfaces to the custom definition only because namespaces can not extended. (namespace JSXInternal extends PreactJSX {...} does not work)

I believe we need a better solution to add custom elements to the typing. Any ideas?

@hbroer
Copy link

hbroer commented Jun 12, 2020

The answer was right here by @somarlyonks. Overwriting module preact/src/jsx instead of preact seems to be the key. I overlooked that comment. ^^

import {JSXInternal} from "preact/src/jsx";

declare module "preact/src/jsx" {
    namespace JSXInternal {
        /* custom IntrinsicElements extension */
    }
}

@jakearchibald
Copy link
Contributor

jakearchibald commented Jun 12, 2020

Whoa, nice! You don't even need to import JSXInternal. I've updated #1180 (comment)

Edit: I'm wrong, you do need the import. An incremental build made things look ok, but they were not.

@jakearchibald
Copy link
Contributor

I think this should go on the preact site, but I couldn't get the dev server working due to preactjs/preact-www#605

@andrewiggins
Copy link
Member

Uh oh - #2575 will break this workaround :( It moves our types from the src folder to the dist folder so you'd have to change your import to "preact/dist/jsx"

@jakearchibald
Copy link
Contributor

It would be good to make Typescript + custom elements first class in preact. As in, documented, and only breaks backwards compatibility in major versions.

@andrewiggins
Copy link
Member

Agreed. I thought we had a test for this in our TypeScript types tests but I seem to be wrong. We should add one to provably demonstrate it works and prevent regressions as well as add documentation like you suggested

@andrewiggins
Copy link
Member

Hmm seems there was a recent fix in TypeScript that may have resolved some of the issues we were seeing that required JSXInternal work arounds. The following fails in 3.9.0-beta but works in 3.9.1-rc

Repo: https://github.com/andrewiggins/cannot-extend-instrinsic-elements-demo/tree/explorations

// module.tsx
export interface ModuleCE {
  instanceProp: string;
}

export interface ModuleCEEvent {
  eventProp: string;
}

export interface ModuleCEAttributes extends preact.JSX.HTMLAttributes {
  ceProp?: string;
  onsomeevent?: (this: ModuleCE, ev: ModuleCEEvent) => void;
}

declare module "preact" {
  namespace JSX {
    interface IntrinsicElements {
      "module-custom-element": ModuleCEAttributes;
    }
  }
}
// app.tsx
import { createElement } from "preact";

export function App() {
  return (
    <div>
      <module-custom-element
        ceProp="ceProp"
        dir="auto" // Inherited prop from HTMLAttributes
        onsomeevent={function (e) {
          console.log(e.eventProp, this.instanceProp);
        }}
      />
    </div>
  );
}

There are over 100 fixed issues between 3.9.0-beta and 3.9.1-rc so haven't yet figured out which contributed the fix but thought I'd share what I've found so far.

@andrewiggins
Copy link
Member

andrewiggins commented Jun 13, 2020

Okay one other update. The pattern below compiles for me all the back to 3.5.3 (which I think is the min version for Preact's types anyway). Thinking this might be the best guidance for us going forward since I think I remember seeing somewhere the TS team want's to move JSX declarations to be based on the jsxFactory (e.g. React for react, commonly h or createElement for preact) though that guidance may be a little out of date - haven't been following too closely 😅

// Works with TypeScript 3.5.3, 3.6.5, 3.7.5, 3.8.3, 3.9.0-beta, 3.9.5
declare module "preact" {
  namespace createElement.JSX {
    interface IntrinsicElements {
      "module-custom-element": ModuleCEAttributes;
    }
  }
}

@andrewiggins
Copy link
Member

Would love people's thoughts about the patterns I used in custom-elements.tsx in #2581!

@marvinhagemeister
Copy link
Member

Reopening until the TypeScript team has published their fix.

@Ciantic
Copy link

Ciantic commented Jun 14, 2020

If you want to embed the type with your component file, you can use declare global, like this (TS 3.9.4):

import { h, render } from 'preact';
import type { FunctionComponent } from 'preact';

// This makes it recgonized by the JSX parts in rest of the application:
declare global {
    namespace preact.createElement.JSX {
        interface IntrinsicElements {
            'oksidi-sharer': { 'share-url': string; };
        }
    }
}
export const OksidiSharer: FunctionComponent<{ shareUrl: string }> = (
    props,
) => {
    return <div>Share this {props.shareUrl}</div>;
};

customElements.define(
    'oksidi-sharer',
    class extends HTMLElement {
        private attachRoot: HTMLElement | ShadowRoot;
        constructor() {
            super();
            this.attachRoot = this.attachShadow({ mode: 'open' });
        }

        connectedCallback() {
            let p = {
                shareUrl: '' + this.attributes.getNamedItem('share-url')?.value,
            };
            render(h(OksidiSharer, p, []), this.attachRoot);
        }

        detachedCallback() {
            render(null, this.attachRoot);
        }
    },
    {},
);

@jakearchibald
Copy link
Contributor

jakearchibald commented Sep 23, 2020

@andrewiggins that doesn't work for me. It means my preact imports stop working:

import { h, Component } from 'preact';

Module '"preact"' has no exported member 'h'.

Edit: This is with TypeScript 4.0.2

@jakearchibald
Copy link
Contributor

ffs, it works if the .d.ts is a module. Adding export {} to the file makes it work. Ugh, I do not understand this part of TypeScript at all.

@TechQuery
Copy link

TechQuery commented Apr 8, 2024

declare global {
  namespace JSX {
    interface IntrinsicElements {
      ['custom-element']: any;
    }
  }
}

which works in React 18, fails in @preactjs 10: idea2app/React-MobX-Bootstrap-ts#12

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Apr 8, 2024

@TechQuery Preact doesn't define the JSX in the global scope like React does, so trying to extend global JSX types won't work. This allows Preact to be used with other JSX-based frameworks at the same time.

declare global {
    namespace preact.JSX {
        interface IntrinsicElements {
            'custom-element': { 'foo': string; };
        }
    }
}

TypeScript playground

@TechQuery
Copy link

@TechQuery Preact doesn't define the JSX in the global scope like React does, so trying to extend global JSX types won't work. This allows Preact to be used with other JSX-based frameworks at the same time.

declare global {
    namespace preact.JSX {
        interface IntrinsicElements {
            'custom-element': { 'foo': string; };
        }
    }
}

TypeScript playground

For library users, there is no multiple JSX configuration fields in single tsconfig.json file. They usually don't have more configuration files in an application project.

For library developers, they can't define JSX types for all kinds of JSX-compatible engines in the world, so this work must be done by users in their projects manually.

It's not a good developer experience in my opinion.

@rschristian
Copy link
Member

For library developers, they can't define JSX types for all kinds of JSX-compatible engines in the world, so this work must be done by users in their projects manually.

There's a pretty small handful of JSX libs that see much use, I don't think asking to add an entry per compatible framework is too much of an ask.

It's not a good developer experience in my opinion.

It's preferable to the (seemingly only) alternative of competing/fighting types, so I'm not quite sure what you'd like to see change.


We've recently added some docs to our site covering this which should hopefully help out here. I'll close this on out as I don't think there's anything actionable for us here (nor does TS seem interested in changing their somewhat React-centric handling of JSX).

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.