solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.44k stars 926 forks source link

solid-styled-components: TypeScript - Base tag props not exposed #147

Closed kavsingh closed 4 years ago

kavsingh commented 4 years ago

Overview

When wrapping with styled, wrapped tag props don't seem to be exposed.


Example repo: https://github.com/kavsingh/tsblank/tree/solid-js-styled npm i and open src/components/count.tsx with type linting enabled:

image

Thanks!

ryansolid commented 4 years ago

Ok, my TypeScript isn't always the best. If someone is interested in submitting a PR that would be appreciated. Otherwise, I will look into this weekend.

high1 commented 4 years ago

I took a look at this, and then at new goober typings - and came up with something like this:

import { css, CSSAttribute } from 'goober';
import { cloneProps, createContext, useContext } from 'solid-js';
import { spread } from 'solid-js/dom';

const ThemeContext = createContext();

export function styled<T extends keyof JSX.IntrinsicElements>(tag: T) {
  return <P extends {}>(
    t: CSSAttribute | TemplateStringsArray | string | ((props: P) => CSSAttribute | string),
    ...p: Array<string | number | ((props: P) => string | number)>
  ) => {
    return (
      props: P & JSX.IntrinsicElements[T] & { className?: string; children?: any; theme?: any }
    ): JSX.IntrinsicElements[T] => {
      const newProps = cloneProps(props);
      props.theme = useContext(ThemeContext);
      Object.defineProperty(newProps, 'className', {
        get() {
          const pClassName = props.className,
            append = 'className' in props && /^go[0-9]+/.test(pClassName!);

          // Call `css` with the append flag and pass the props
          const className = css.apply({ target: this.target, o: append, p: props }, [
            typeof t === 'function' ? t(props as P) : t,
            ...p.map((prop) => (typeof prop === 'function' ? prop(props as P) : prop)),
          ]);

          return [pClassName, className].filter(Boolean).join(' ');
        },
        configurable: true,
        enumerable: true,
      });

      const el = document.createElement(tag);
      spread(el, newProps);

      return (el as unknown) as JSX.IntrinsicElements[T];
    };
  };
}

Usage:

interface ButtonProps {
  height?: number;
}

const TestDiv = styled('div')<ButtonProps>`
  border-radius: 20px;
  height: ${({ height = 100 }) => height}px;
  background: red;
`;

const TestAnchor = styled('a')`
  background: blue;
`;

<TestDiv onClick={() => alert('test')} />
<TestAnchor href="#" />

Type of TestDiv is JSX.HTMLAttributes, I'm not sure if that;s quite correct. But it worked when I tested it here. Now, the string for tag is type checked, so you can't call it like styled('xyz'). Also, it should pick up props from the JSX.IntrinsicElements[T], so assigning href to TestDiv would fail. And the ButtonProps type checks. Used typings from goober for CSS function, rather than convert to [any, any[]] - but this can be changed.

ryansolid commented 4 years ago

Hmmm.. I guess that checks because from a JSX perspective it's only ever the attributes that are being looked at. This whole area seems so complicated. Once I get the JSX types in Solid I suppose this will be easier to wire up. In general, I'm so confused over Components even referencing JSX types when they don't need to use JSX. Preact often uses h directly or htm these days. Are JSX types throughout their type declarations. ... And yes that seems to be the case. I mean sure they reference their element type in most places but when it comes down to intrinsic elements. I guess I shouldn't worry about this at all given the types will be shipped with Solid.

high1 commented 4 years ago

Looking at the new goober typings for styled, i see that they've imported React typings:

interface StyledFunction {
        // used when creating a styled component from a native HTML element
        <T extends keyof JSX.IntrinsicElements, P extends Object = {}>(tag: T): Tagged<
            JSX.LibraryManagedAttributes<T, JSX.IntrinsicElements[T]> & P
        >;

        // used to extend other styled components. Inherits props from the extended component
        <PP extends Object = {}, P extends Object = {}>(tag: StyledVNode<PP>): Tagged<PP & P>;

        // used when creating a component from a string (html native) but using a non HTML standard
        // component, such as when you want to style web components
        <P extends Object = {}>(tag: string): Tagged<P & Partial<JSX.ElementChildrenAttribute>>;

        // used to create a styled component from a JSX element (both functional and class-based)
        <T extends JSX.Element | JSX.ElementClass, P extends Object = {}>(tag: T): Tagged<P>;
    }

My implementation covers the first case, and I've omitted JSX.LibraryManagedAttributes because I guess there's no default props in Solid like in React. https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-0.html#support-for-defaultprops-in-js Looking at this, I see that there are 3 more cases, but I'd be more concerned that goober now has global JSX namespace due to React typings import. This is a case that is not covered here:

const HelloComponent: FC<{ name: string }> = ({ name }) => <h1>Hello, {name}</h1>;
const StyledHello = styled(HelloComponent)`
  color: blue;
`;

and FC would be just

type PropsWithChildren<P> = P & { children?: JSX.Element };
  interface FunctionComponent<P = {} & JSX.HTMLAttributes<unknown>> {
    (props: PropsWithChildren<P>): JSX.Element | null;
  }
  type FC<P = {} & JSX.HTMLAttributes<unknown>> = FunctionComponent<
    P & JSX.HTMLAttributes<unknown>
  >;
high1 commented 4 years ago

Excuse my ignorance, but I saw multiple times that Babel is the only option for doing the code optimization. If you would introduce a JSXfactory - a function that wold render DOM elements from JSX, couldn't it also be an option? The reason why I'm asking is - if that is possible, this could largely increase compatibility with existing libraries - for instance, goober setup could be used with this function to make goober work correctly, and thus eliminate the need for a custom wrapper. Also, global JSX export would not be needed. But again, it's just an uneducated guess...

ryansolid commented 4 years ago

Hey I do have a JSXFactory function in solid-js/h which is there purely for compatibility since I have a more performant non-compiled approach in solid-js/html. So it's doable, I just don't make that the main solution since it can't be optimized in the same way. Basically the big benefit here is the DOM Expression's compiler. Without it you might as well be using a Virtual DOM since you have to do all the same work, and put a reactive system on top. The compiler is unique angle that reactive libraries can take to remove the guesswork. An HyperScript factory always needs to ask the question with new data of what it's doing. It doesn't have the ability to create DOM nodes on mass. It requires manual indication of what's reactive or not. It has to do all these checks at runtime and at a per element granularity. Atleast Tagged Template Template literals (solid-js/html) can analyse the whole template and cache the execution. HyperScript executes inside out:

// which h function executes first?
h("div", { id: "main" }, [
  h("div", "hello"),
  h("div", h("span", () => state.name))
])

This lends to creating the structure first then walking over it again to build something. Now you could probably find a way using wrappers to cache this information similar to Tagged Template Literals after the first execution (and I don't mean reactive execution since that never runs again, more like the next row). But it requires new wrappers syntax that would break compatibility anyway.

I think this is what Rich Harris means when he says Svelte can't use JSX. He isn't talking about JSX, he means HyperScript. I have gone through the effort of making Solid HyperScript compatible and it is pretty performant but it just can't be the de facto approach. See my article The Fastest Way to Render the DOM.

high1 commented 4 years ago

So there is no possibility of a hybrid way out of of this? To make this JSX factory and the compilation step work together, and exchange information? The goal here is to reuse the previous work - because if this is not a possibility, I fear that there will be an issue raised for each popular JSX library going forward. Taking a thorough read of the article, thanks.

ryansolid commented 4 years ago

I mean there is a hybrid. Sort of if you ignore reactivity. But you can't really ignore reactivity. In some cases it will just work but it will never be the optimal approach. I guess I never viewed things as JSX libraries but rather React libraries or React-like libraries. Most things in this space are never going to work simply because they expect a React like update cycle regardless of what the syntax is. Its the same reason Solid will never be a swap in for Function Components with hooks. The update cycle is backwards.

Goober does attempt to make a general JSX approach but it is for the most part just using HyperScript. HyperScript is innately VDOM. It basically constructs a virtual tree. Stuff like the way it executes inside out doesn't lend to single pass approaches. I've definitely made something that can work in these sort of situations but it won't be 100% optimal. JSX on the other hand is this: https://facebook.github.io/jsx/. No more not less. People make HyperScript libraries not JSX libraries from my perspective because they work without JSX. HyperScript is just the most common JSX target.

So I guess the best way to think of this is baseline you shouldn't expect these libraries to be any more compatible than they'd be with say Svelte. I do have escape hatches if you need to support these short term until better solutions comes around. In general JSX just has too many benefits as a DSL which is why I chose it. Clear AST, easy use with existing tools like Babel, TypeScript support, Syntax Highlighting, familiar syntax, defined composition model. Basically JSX is great so why re-invent the wheel?

EDIT: I guess you wondering if the compiler can identify this situation. Not easily I don't think. Like how would know whether to generate optimal code vs HyperScript. Mostly that it still takes some consideration here, as a general HyperScript still needs reactive wrapping consideration. I mean someone could use Babel's React Transform JSX instead of DOM expressions and target Solid's HyperScript. But depending on the library it still might still not just work. Or it would be really unoptimal. We create real DOM nodes instead of Virtual Nodes so if it found itself in an update cycle you might be recreating DOM nodes over and over. If we wanted to loosen the granularity we'd need a mechanism to do diffing and if we wanted to use stock HyperScript we'd essentually need to create VDOM pockets to diff. It's conceivable but I hope you understand why that wouldn't be the default approach.

high1 commented 4 years ago

Agreed. Unfortunately, expectations go beyond that... OK,I guess we'll have to wait for thing to unwind.

ryansolid commented 4 years ago

Yeah I guess you can appreciate more why I realized I needed to make my own library wrappers. I sort of didn't want to take it on, but realistically I need some projects out there so people can get started. But there will be gaps at first, especially when I'm implementing things that I don't have a lot of experience with. It's a bit of a slog as there are always a couple cases I miss. Which is largely why I haven't been comfortable dropping 1.0 yet. But as more people have been using and reporting issues the "solid" this is all getting.

high1 commented 4 years ago

Yeah, noted... That's quite a lot take on - and it's not the way forward. It's just not doable. I mean, I get the impression that it would be easier to rewrite goober's styled function than to write typed compatible implementations in TypeScript.

ryansolid commented 4 years ago

I believe I've done something reasonable here based on @high1 suggestions. As far as I can tell this worked in my tests. Please reopen if still an issue. I'm happy to report this is fixed in v0.18.0.