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

Bug in generic types for universal render target? #277

Open
Alloyed opened this issue Oct 6, 2023 · 5 comments
Open

Bug in generic types for universal render target? #277

Alloyed opened this issue Oct 6, 2023 · 5 comments

Comments

@Alloyed
Copy link

Alloyed commented Oct 6, 2023

https://github.com/ryansolid/dom-expressions/blob/main/packages/dom-expressions/src/universal.d.ts

Hi, I'm trying to implement a custom renderer that does not target the dom at all (!). As far as I can tell, to do this I need to

A. replace JSX.Element with a variant that does not reference dom nodes ✅
B. call createRenderer<MyCustomNode>.

Doing this creates types with (roughly) this signature:

type JSX.Element = MyCustomNode | JSX.Element[] | null | undefined; //... etc
const render: (code: () => MyCustomNode, node: MyCustomNode) => () => void

The problem here is that the component definition () => MyCustomNode appears to be different type to JSX.Element. The (DOM) definition of JSX.Element includes all sorts of things that aren't just HTMLElement, but when implementing my renderer I only wrote custom methods in terms of my custom dom node type, and not the array/null/other things.

What I naively would've expected is something like this signature:

interface Renderer<NodeType, JsxElementType> {
    const render: (code: () => JsxElementType, node: NodeType) => () => void;
}

Is the type declaration wrong here, or should I be implementing my renderer to handle all those extra types, too?

@jpdutoit
Copy link
Contributor

I agree the type definition is wrong here. It should allow any JSX.Element return. Casting it to your replacement works fine in my tests.

@jpdutoit
Copy link
Contributor

I believe it should be the following:

interface Renderer<NodeType> {
    const render: (code: () => UniversalJSXElement<NodeType>, node: NodeType) => () => void;
}

type UniversalJSXElement<NodeType> =
  | NodeType
  | Array<UniversalJSXElement<NodeType>> 
  | (string & {})
  | number
  | boolean
  | null
  | undefined

@Alloyed
Copy link
Author

Alloyed commented Oct 16, 2023

ok, found another issue. It's related enough that I don't want open a second ticket for it. Existing docs for custom renderers suggest exposing control flow components like so:

// Forward Solid control flow
export { ErrorBoundary, For, Index, Match, Show, Suspense, SuspenseList, Switch } from 'solid-js';

Here's what the declaration looks like for <For/>, when you do this

import type { JSX } from "solid-js/types/jsx";
export declare function For<T extends readonly any[], U extends JSX.Element>(props: {
    each: T | undefined | null | false;
    fallback?: JSX.Element;
    children: (item: T[number], index: Accessor<number>) => U;
}): JSX.Element;

This means all our control flow primitives are using the definition of JSX provided by solid (aka the dom version) instead of our non-dom version. So likewise, I will probably work around this by replacing these declarations for the time being

EDIT: actually, it's likely that any third party libraries, even if they don't depend on the dom, will be using import("solid-js").JSX too. so maybe I need to be augmenting that type, rather than creating my own independent type.

@jpdutoit
Copy link
Contributor

jpdutoit commented Oct 18, 2023

Yeah, this is a real problem. I work around it by basically copy-pasting all those type definitions

import { For as UntypedFor} from "solid-js"

export const For = UntypedFor as <T extends readonly any[], U extends JSX.Element>(props: {
  each: T | undefined | null | false
  fallback?: JSX.Element
  children: (item: T[number], index: Accessor<number>) => U
}) => JSX.Element

It works, but I end up constantly fighting the VSCode auto-importer to import the methods from the correct file. So, ideally the definitions should be updated to be NodeType agnostic. I think those definitions live in the main solid repo, so an issue there would make sense.

I think a wider discussion is needed here. Currently the developer experience in Typescript is not great.

@chiefcll
Copy link

Thanks for the info - we're working on adding Types to our project which is universal renderer... https://github.com/lightning-js/solid
@frank-weindel

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

No branches or pull requests

3 participants