feat: Add ElementRef type to compat

[rschristian]

Closes #4481

For comparison: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b5dc32740d9b45d11cff9b025896dd333c795b39/types/react/index.d.ts#L221-L235

Can confirm #4548 cleared the way for this -- like a total doofus I created a new vite project and was wildy confused to see the same old error re: can't assign null | undefined to VNode<any> as I had only copied over this particular change. Eventually the issue dawned on me, copied over the new types, and it worked like a treat!

[rschristian]

We didn't originally have ElementType when I added ComponentPropsWithRef but someone kiindly added it a few months ago. It's worth switching this over, helps ensure things are kept in-sync.

For comparison: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b5dc32740d9b45d11cff9b025896dd333c795b39/types/react/index.d.ts#L1687-L1689

[coveralls]

coverage: 99.617%. remained the same
when pulling ad0e551 on types/element-ref
into daa124a on main.

[JoviDeCroock]

How does this behave in 5.0?

[rschristian]

That's a great question. Will investigate further

[rschristian]

Ignoring our resolution issues for the moment, this seems to work as we need? It does produce a type error in the compat declaration file, but in 5.0 & 5.1, the user is presented with a seemingly functional & identical type. This would require skipLibCheck, but most of our users are probably doing so anyhow (our Vite templates all do this by default).

Copy/pasting the shadcn examples given in the issue, there doesn't seem to be a user-facing problem, it's just that the generic doesn't satisfy. I'm not well enough informed to say whether this is a problem or not though.

[jakebailey]

I'm currently trying to chase down why so much of DT is broken right now due to preact; I can definitely say that you do not want to end up a situation where the types do not check without skipLibCheck. DefinitelyTyped is by definition declaration files which can only be checked without skipLibCheck so it's critical that all packages there and their dependencies contain no errors.

[jakebailey]

If you don't want to support TS 5.0, then theoretically we can bump all of the packages on DT up to a minimum of TS 5.1, though that may of course break someone.

[rschristian]

you do not want to end up a situation where the types do not check without skipLibCheck

This has always been the case in Preact due to the limited aliasing mechanisms we have in TS; we don't provide every single React type, but do need consumers to use our types over @types/react, so skipLibCheck seems like it'll be necessary forever?

As for issues on DefinitelyTyped we'd love to know more -- last release we did a fair few type changes (longstanding issues) and while we were a bit worried about doing it, we found no issues ourselves. Is it perhaps the added strictness in per-element types (#4546) or related to FunctionComponent now returning a ComponentChild in TS 5.1 (#4548)?

[rschristian]

Re:skipLibCheck, I think I misunderstood you.

We've held out on merging this due to that type error, as it seems TS won't use typeVersions or the equivalent in our export map when resolving submodules like in this case. We need preact/compat to resolve either the TS 5.0 preact types or the 5.1+ types, though haven't been able to figure it out. Using the module specifier (i.e., preact) doesn't seem to work. Would we need to make TS 5.0 & 5.1+ splits for everything that consumes the split types too?

[jakebailey]

As for issues on DefinitelyTyped we'd love to know more -- last release we did a fair few type changes (longstanding issues) and while we were a bit worried about doing it, we found no issues ourselves.

See: https://github.com/DefinitelyTyped/DefinitelyTyped/actions/runs/12009667461

Most failures here are due to those changes, it seems.

[jakebailey]

We've held out on merging this due to that type error, as it seems TS won't use typeVersions or the equivalent in our export map when resolving submodules like in this case. We need preact/compat to resolve either the TS 5.0 preact types or the 5.1+ types, though haven't been able to figure it out. Using the module specifier (i.e., preact) doesn't seem to work. Would we need to make TS 5.0 & 5.1+ splits for everything that consumes the split types too?

typesVersions is a little annoying to use since it's largely an external config, not an internal one. The node types have full checking but it's nontrivial.

It may be easier to just drop older versions

[rschristian]

That's unfortunate. Last time we bumped TS versions a lot of external projects had considerable issues, especially within the Angular ecosystem which I believe locks TS versions per-release. Preact, being a rather great option for widgets, sees a fair number of wrappers created around Preact components for use elsewhere.

As such, I'm not a fan of touching that minimum, especially to raise it to something so new.

[jakebailey]

The minimum version supported by DT is 5.0; 5.1 is not that far off I wouldn't think.

But, I haven't dug more deeply into the problems here (but plan to do DT is unblocked)

[rschristian]

As for the DefinitelyTyped errors, I'm not sure what we're doing wrong here? That error would be "expected" if a TS 5.0 (or older) consumer was pulling our new types, but that shouldn't be happening to my knowledge. Our config should hopefully be directing those consumers to slightly different types:

preact/package.json

Lines 12 to 16 in bd14437

	"typesVersions": {
	"<=5.0": {
	".": ["./src/index-5.d.ts"]
	}
	},

preact/package.json

Lines 18 to 21 in bd14437

	".": {
	"types@<=5.0": {
	"types": "./src/index-5.d.ts"
	},

As for what versions DT supports, that doesn't really impact us here. We'd prefer support much further back than just 5.0 for our own repos I think.

[JoviDeCroock]

Hey @jakebailey,

Thanks for dropping by! I am a bit confused how we broke this many tests in DefTyped, are these dependencies using Preact under the hood or are there explicit tests of our types package?

I've been looking into a few of these, I saw ember passing by which is definitely a React dependent. When we made the change to have variable support for < 5.1 and >= 5.1 we took this from @types/react as something in the JSX types of 5.1 was seemingly disruptive for the ecosystem.

As seen here - we have taken this PR as inspiration for #4548. Now I didn't feel 100% confident in the whole typeVersions directive in package.json myself so happy to change things. Another thing is that maybe this issue is caused by people importing preact/compat which might not leverage the typeVersions from the root-package well enough?

Dropping support for TS < 5.1 is a no-go, we have had grievances with this in the past with this as mentioned by Ryan and I would prefer for us not to do this again. We've been stable for a long time and doing breaking changes due to TypeScript in patch/minor versions is something I'd avoid as a service to our community.

What can we do to help you here?

[jakebailey]

I was busy fixing other DT issues and didn't get to looking at this one; I plan to look harder at this come tomorrow.

Dropping support for TS < 5.1 is a no-go, we have had grievances with this in the past

By nature of DT itself dropping 5.0, it's very possible that 5.0 will break for someone in the ecosystem anyway (via other types packages), but I'm sure we'll be able to find something that works anyway. Two years of types support is pretty generous when the alternative for downstream projects is to... Not update their deps? (Surely if they can update preact, they could update a devDep like TypeScript, but, I digress)

[jakebailey]

I overcounted; the preact related failures are just:

Error in mdx
Error: 
/home/runner/work/DefinitelyTyped/DefinitelyTyped/types/mdx/preact-tests.tsx
   57:2   error  [email protected] tsconfig.preact.json, 5.1 tsconfig.preact.json, 5.2 tsconfig.preact.json, 5.3 tsconfig.preact.json, 5.4 tsconfig.preact.json, 5.5 tsconfig.preact.json, 5.6 tsconfig.preact.json, 5.7 tsconfig.preact.json, 5.8 tsconfig.preact.json compile error: 
Type '{}' is not assignable to type 'IntrinsicAttributes & SVGAttributes<SVGSymbolElement> & ObjectHTMLAttributes<HTMLObjectElement> & ... 158 more ... & SemanticsMathMLAttributes<...>'.
  Property 'datetime' is missing in type '{}' but required in type 'DelHTMLAttributes<HTMLModElement>'  @definitelytyped/expect
   57:2   error  [email protected] tsconfig.preact.json compile error: 
'Div' cannot be used as a JSX component.
  Its element type 'ComponentChildren' is not a valid JSX element.
    Type 'undefined' is not assignable to type 'Element | ElementClass | null'                                                                                                                                                                                                                                                                                                                         @definitelytyped/expect
   73:13  error  [email protected] tsconfig.preact.json, 5.1 tsconfig.preact.json, 5.2 tsconfig.preact.json, 5.3 tsconfig.preact.json, 5.4 tsconfig.preact.json, 5.5 tsconfig.preact.json, 5.6 tsconfig.preact.json, 5.7 tsconfig.preact.json, 5.8 tsconfig.preact.json expected type to be:
  HTMLAttributes<HTMLAnchorElement>
got:
  AnchorHTMLAttributes<HTMLAnchorElement>     

Error in preact-custom-element
Error: 
/home/runner/work/DefinitelyTyped/DefinitelyTyped/types/preact-custom-element/preact-custom-element-tests.tsx
  4:7  error  [email protected] compile error: 
Type '(props: any) => Element' is not assignable to type 'FunctionalComponent<any>'.
  Call signature return types 'Element' and 'VNode<{}> | null' are incompatible.
    The types of 'type' are incompatible between these types.
      Type 'string | ComponentType<any>' is not assignable to type 'string | ComponentType<{}>'.
        Type 'ComponentClass<any, {}>' is not assignable to type 'string | ComponentType<{}>'.
          Type 'ComponentClass<any, {}>' is not assignable to type 'ComponentClass<{}, {}>'.
            Types of property 'contextType' are incompatible.
              Type 'preact.Context<any> | undefined' is not assignable to type 'import("/home/runner/work/DefinitelyTyped/DefinitelyTyped/types/preact-custom-element/node_modules/.pnpm/[email protected]/node_modules/preact/src/index-5").Context<any> | undefined'.
                Type 'preact.Context<any>' is not assignable to type 'import("/home/runner/work/DefinitelyTyped/DefinitelyTyped/types/preact-custom-element/node_modules/.pnpm/[email protected]/node_modules/preact/src/index-5").Context<any>'.
                  The types returned by 'Consumer(...)' are incompatible between these types.
                    Type 'ComponentChildren' is not assignable to type 'VNode<{}> | null'.
                      Type 'undefined' is not assignable to type 'VNode<{}> | null'  @definitelytyped/

Error in preact-virtual-list
Error: 
/home/runner/work/DefinitelyTyped/DefinitelyTyped/types/preact-virtual-list/preact-virtual-list-tests.tsx
  19:7  error  [email protected] compile error: 
Type '(props: any) => h.JSX.Element' is not assignable to type 'FunctionalComponent<any>'.
  Call signature return types 'Element' and 'VNode<{}> | null' are incompatible.
    The types of 'type' are incompatible between these types.
      Type 'string | ComponentType<any>' is not assignable to type 'string | ComponentType<{}>'.
        Type 'ComponentClass<any, {}>' is not assignable to type 'string | ComponentType<{}>'.
          Type 'ComponentClass<any, {}>' is not assignable to type 'ComponentClass<{}, {}>'.
            Types of property 'contextType' are incompatible.
              Type 'preact.Context<any> | undefined' is not assignable to type 'import("/home/runner/work/DefinitelyTyped/DefinitelyTyped/types/preact-virtual-list/node_modules/.pnpm/[email protected]/node_modules/preact/src/index-5").Context<any> | undefined'.
                Type 'preact.Context<any>' is not assignable to type 'import("/home/runner/work/DefinitelyTyped/DefinitelyTyped/types/preact-virtual-list/node_modules/.pnpm/[email protected]/node_modules/preact/src/index-5").Context<any>'.
                  The types returned by 'Consumer(...)' are incompatible between these types.
                    Type 'ComponentChildren' is not assignable to type 'VNode<{}> | null'.
                      Type 'undefined' is not assignable to type 'VNode<{}> | null'  @definitelytyped/expect

You can see that the latter two ones are resolving index-5, but what isn't good is that it seems to be cross comparing the <=5.0 types and the >=5.1 types. --explainFiles says:

node_modules/.pnpm/[email protected]/node_modules/preact/src/index.d.ts
  Imported via './index' from file 'node_modules/.pnpm/[email protected]/node_modules/preact/src/jsx.d.ts' with packageId 'preact/src/[email protected]'
  File is CommonJS module because 'node_modules/.pnpm/[email protected]/node_modules/preact/package.json' does not have field "type"

And indeed jsx.d.ts imports index directly. Theoretically the way to fix this would be to not use a relative import there, but instead import from the public name preact such that it goes through typesVersions resolution. Alternatively, you can define two separate, different files that declare just the differing type, then import that from index.d.ts instead. That's sort of the problem with typesVersions, it really makes local references tricky.

The mdx issue seems to be a different one entirely, breaking on all versions.

I'm not sure if this PR is the right place to chat about this, happy to talk wherever.

[rschristian]

The datetime/mdx issue appears to be (in part) the bug fixed in #4570, so some of it can be ignored at least.

And indeed jsx.d.ts imports index directly.

Ah damn, I missed that. If using preact would have TS resolve through the typeVersions resolutions, that would be the bug I was running up against here. Will try that out & get everything normalized then.

I'm not sure if this PR is the right place to chat about this, happy to talk wherever.

Wherever easiest/most comfortable for you, we appreciate you looking into it & helping us with this! Separate issue, discussion, we have a slack, or just keep everything here, wherever.

[jakebailey]

Regarding DefinitelyTyped/DefinitelyTyped#65126 and #4548, note that the former duplicated the entire package, not just one file.

Doing the split by file is more challenging, and only works if you can carefully control the way files are loaded. We were able to do this for a recent Node change, for example, but their types are global so it's not super hard to get right given everything starts from the root index.d.ts, which we can duplicate with slight changes to references.

In your case, the solution may be to just not use relative paths in these handwritten files, which should just work? There aren't many files to double check.

Otherwise, I would really only suggest duplicating everything as part of some build step, or maybe, introducing a separate compat package preact depends on for compat, but that seems like a bad idea for a dependency free library 😄

If you have a change, I'm super happy to check it locally. You can also clone DT and then point preact at a local dir and run tests (though be careful to avoid installing the entire repo of 8000+ packages).

[rschristian]

Okay so I think #4576 should correct our types here. As you suggested, it goes through and swaps out all references to the now-split types with a bare preact specifier. Seems to work correctly, fixing the issue we had in this PR (where the default, 5.1+ types were pulled in when they weren't supposed to be).

Hopefully that gets DefinitelyTyped working again.

[jakebailey]

#4576 does seem to work to fix things. mdx is also fixed after the datetime thing, though needing a test change to account for HTMLAttributes<HTMLAnchorElement> -> AnchorHTMLAttributes<HTMLAnchorElement>.

I've commented on that PR, though, since it seems like the absolute import is not working out.

[rschristian]

though needing a test change to account for HTMLAttributes<HTMLAnchorElement> -> AnchorHTMLAttributes<HTMLAnchorElement>.

Ah yeah, I think that should be the only intentional change now that we're offering stricter types. HTMLAttributes used to be a mega interface of all possible attributes -- convenient for us to maintain but not so type safe.

Glad that PR otherwise fixes things though!

[rschristian]

Now looks good. Both 5.0 & 5.1+ show no errors in /compat/index.d.ts now that the type resolution is set up for all consumers of /src/index.d.ts (/src/index-5.d.ts). Gonna merge this in then.