RFC: Remove FunctionElement from JSX.Element types

[ryansolid]

Summary

This RFC looks for feedback on a change to restrict TypeScript JSX in a way that will make the journey better for most developers but make things more forced for advanced cases. By removing Function Element when and how to use functions in Solid's JSX would be greatly simplified, and produce much more idiomatic and predictable code.

Background

I have struggled with this forever and I realize a lot of it comes from my background of using both JavaScript in a very JavaScript way and having experienced in typed languages that are not TypeScript. Our types to the best of my efforts reflect everything that could be done with our syntax, but not what makes sense to do.

There are details in our runtime that we need to support but are confusing to developers as there are conditions where passing functions works as expected and others that don't. However, from a DSL design standpoint, we should only be passing functions that receiving JSX Element is expecting. This confusion has caused a number of issues submitted that are basically abusing a mechanism isn't intended.

Proposal

We remove FunctionElement from the JSX.Element types. This has several implications but I will show some of the most common ones.

Authoring

First authoring components. You will not be able to return a function or Memo without casting it. So do that:

function MyComp(props) {
-  return createMemo(() => props.count * 2)
+  return createMemo(() => props.count * 2) as unknown as JSX.Element
}

Or return a fragment

function MyComp(props) {
-  return createMemo(() => props.count * 2)
+  return <>{props.count * 2}</>
}

Usage

1. Functions as children JSX native elements now will error

 // Type '() => JSX.Element' is not assignable to type 'JSX.Element'
-  <div>{() => <span />}</div>
+  <div><span /></div>

const [count, setCount] = createSignal(0);

// Type '() => number' is not assignable to type 'JSX.Element'
-  <div>{count}</div>
+  <div>{count()}</div>

This makes it much clearer from types that you should be calling signals as functions and that arbitrary functions inlined are not expected. The big plus of this is people coming from other non-signal frameworks will immediately understand they aren't in Kansas anymore and push them towards calling the function. Something already necessary for native prop bindings.

2. Component children/renderBodies will only expect function children if defined

function MyLayout(props: { children: JSX.Element }): JSX.Element;

function MyFor<T, U extends JSX.Element>(props: { each: T[],  children: (item: T) => U): JSX.Element;

// valid
<MyLayout>Hello</MyLayout>
<MyLayout><p>Hello</p></MyLayout>
<MyLayout>{name()}</MyLayout>
<MyLayout>{name() && <p>Hello</p>}</MyLayout>
<MyLayout>{(() => {
  // some work
  return <p{name()}</p>
})()}</MyLayout>
<MyLayout>{untrack(() => {
  // some work
  return <p>{name()}</p>
})}</MyLayout>
<MyFor each={users()}>{(user) => <div>{user.name}</div>}</MyFor>

// invalid
<MyLayout>{() => <p>Hello</p>}</MyLayout>
<MyLayout>{() => "Hello"}</MyLayout>
<MyLayout>{name}</MyLayout>
<MyLayout>{() => name() && <p>Hello</p>}</MyLayout>
<MyFor each={users}>{(user) => <div>{user.name}</div>}</MyFor>
<MyFor each={users()}><div>Not a Function</div></MyFor>

This removes any expectations of just randomly passing functions to Components not expecting them.

Alternative

Instead of removing the Function Element give it a typescript unique symbol that would be implemented on Solid internals like our Accessor Type so it specifically allows Signals and Memos to be used directly. This removes the authoring burden. However, it produces less clear error messages complaining about missing symbols and it still allows for inconsistency in a number of places where you can pass a Signal directly versus a function.

Impact

This being part of the JSX types will impact every project once adopted. There will be new TS errors but no functionality will change. That being said this should mainly impact library authors and utilities, and can be solved with a backwards-compatible cast. as unknown as JSX.Element

Adoption

Ideally since TypeScript itself has set the precedence I'd like to introduce this change in a minor version. Mostly likely 1.7. This puts us in a good place on semantics to support the migration to Solid 2.0 implementation updates.

[atk]

Maybe we can use types to our advantage here and make it so that attempts to use function components will emit a helpful message in the type error.

[tannerlinsley]

💯 I was bit by this when I started learning and still am sometimes today. Seeing more errors for when I'm doing something wrong and requiring more explicitness on templating returns is a good tradeoff

[otonashixav]

To add to the alternative, we can make the error clearer, but never as clear as a "native" typescript error. The name of the missing key is in the error message, so if we named the symbol e.g. Only_accessors_allowed_as_JSX_Function_Elements that would show:

Type '() => any' is not assignable to type 'Element'.
  Property '[Only_accessors_allowed_as_JSX_Function_Elements]' is missing in type '() => any' but required in type 'FunctionElement'.

We could go further and use a string, though I worry that someone somewhere would try and actually assign the property.

That said I think outright removing it makes more sense.

[trusktr]

This sounds good. The only thing I don't like is breaking changes in minor releases.

I personally would go to 2.0, and then not hesitate to go to 3.0, 4.0, etc, as needed and organically without too many breaks at once.

As a user I would prefer a clear migration guide for each breaking change, and I would also like less breaking changes at a time (more major bumps), so that I have a clear path, and so that the version numbers have real meaning.

The worse is installing something in a project, then the next npm install breaks it. It is absolutely not ideal.

[mikeplus64]

I posted on Discord as well but I want to post here as well just for visibility. Unfortunately I don't keep up to date that frequently on updates here so wasn't aware of this change until just now.

solid/packages/solid/src/render/flow.ts

Line 141 in 57f4d51

) as unknown as JSX.Element;

<== this is deeply unsatisfying - now basically all abstractions, even those as simple as Show, rely on an unsafe type cast. Either it works and should type-check, or it doesn't work, and shouldn't type-check. It shouldn't work and also not type-check (as now Show and any other component that uses control flow must now do).

My non-library website codebase now has 256 type errors to fix, all by adding this unsafe cast. It isn't just library authors that are affected by this change (although they are too), consider a simple component that just displays the min:sec until a target date/time:

https://gitlab.com/_mike/kuachicups/-/blob/master/client/src/shared/ui/Time.tsx#L25

It doesn't want to affect HTML layout any more than it has to, so it just returns a string internally. This now no longer works.

I think adding a symbol to the createMemo/createSignal types would be the best way to do it. Ultimately, types should be honest first, and producing-nice-error-messages second. If you make the key for the symbol basically an entire paragraph explaining what went wrong, that will probably be informative enough; like

export interface Memo<T> {
  readonly [`
  == solid-js error ==
  ====================
  Functions must be wrapped in @createMemo@ to be renderable.

  For more information see https://github.com/solidjs/solid/discussions/1554
  `]: unique symbol;
  (): T;
}

[AlexTMjugador]

I've stumbled across a scenario similar to @mikeplus64's today in an end-user application, in my case for a context that returns localized string accessors for use by other components.

My application needs to work with the WebKit webview bundled with Linux distributions, and trivially replacing occurrences of <Component>{l10n("key")}</Component> with <Component>{l10n("key")()}</Component> triggers an infinite busy loop that renders the application unusable, and doesn't happen in Firefox or Chrome. I've tried to debug it, but the WebKit devtools are unusable when this happens, and even after reviewing and refactoring my the affected components several times over, I can't get enough information to even file a bug report in the appropriate project.

Adding the as unknown as JSX.Element cast works, but as Mike put it, it's a "deeply unsatisfying" solution, especially given that this localization string accessor is used almost everywhere in the application.

Can TypeScript definitions be honest first again, please? To be frank, the minor ergonomic improvements provided by this change won't do much for newbies. Debugging weird issues with bad error messages is the bread and butter of realistic JavaScript development anyway, and in this case the error message perhaps doesn't have to be that bad, as Mike pointed out.

[clinuxrulz]

Was there a work around for this: as unknown as JSX.Element for FunctionElement ? It is not a very appealing solution. I noticed it just now when updating from SolidJS 1.5 to 1.7.
What if a new element wrapper type was created for wrapping up those FunctionElements

E.g.

function MyComp(props) {
-  return createMemo(() => props.count * 2)
+  return createFnElement(createMemo(() => props.count * 2))
}

Then make FunctionElement a class instead of an interface.

export class FunctionElement {
    readonly fn: () => Element;

    constructor(fn: () => Element) {
        this.fn = fn;
    }
}

export function createFnElement(fn: () => Element): FunctionElement {
    return new FunctionElement(fn);
}

I suppose the issue is ensuring createMemo is used for FunctionElement, in which case createMemo may need to return something that can be called like a function, but have a different type than a function.

E.g.

// This class can be the return type from createMemo()
class MemoResult<A> extends Function {
    private fn: () => A;

    constructor(fn: () => A) { 
        super();
        this.fn = fn;
        return new Proxy(this, {
            apply: (target, that, args) => target.__call__(),
        });
    }

    __call__(): A {
        return this.fn();
    }
}

let test = new MemoResult(() => 5);
let x = test();

Edit: Actually, that MemoResult does not end up having the correct calling type signature (x has type any). I'll just have to bite the bullet and update my own code for version 1.7.

[clinuxrulz]

I am quite happy with the return a fragment solution. There is not need to do the ugly typecasting anywhere, even when using a memo.

function MyComp(props) {
-  return createMemo(() => props.count * 2);
+  let x = createMemo(() => props.count * 2);
+  return <>{x()}</>;
}