solidjs / solid-styled-components

A 1kb Styled Components library for Solid
MIT License
286 stars 27 forks source link

Inconsistency between styled's implementation and types #37

Closed 100terres closed 1 year ago

100terres commented 2 years ago

Issue

The types allow for multiples arguments when styling a component. The following is correct based on the types.

const Component = styled('div')(
  { color: "blue" },
  (props) => ({ color: props.theme.palette.primary }),
  () => ({ color: "pink" }),
  // ...
);

We could expected that a textNode passed to Component would be pink, but with the current implementation the result is blue (the first argument).

It's due to the following lines:

https://github.com/solidjs/solid-styled-components/blob/dec99a0a51e1eaeb8783390291c09e4e436f046b/src/index.js#L47-L50

By looking at this code we could expected that the textNode would be blue. When we do the apply the second argument expected an array of arguments _(see: apply doc)_. Here the args variable represents all the argument passed to style the div. It's like if we were doing the folowing:

// to simplify the `this` "assignment" isn't part of the example
css(
  { color: "blue" },
  (props) => ({ color: props.theme.palette.primary }),
  () => ({ color: "pink" }),
  // ...
);

The issue is that the css method from goober only take one argument. Everything after the first argument is ignored.

https://github.com/cristianbote/goober/blob/b2781bd9d76f5b386e50b5916216430f4532662c/src/css.js#L5-L9

/**
 * css entry
 * @param {String|Object|Function} val
 */
function css(val) {

Secondary issue: We could also take the time to improve the types, because currently there's no way to mark the theme prop as not optional an the as prop types should probably be ValidComponent instead of string | number | symbol | undefined


Proposed solution

Only tag function should be allowed to have multiple arguments

There's a way to make the tag function work while restricting regular function call to one argument. Based on what's happening in goober's css function, here's what the types could look like (without the generic type for the props):

import type { ValidComponent } from "solid-js";

type StylesGenerator = (props: {}) => CSSAttribute | string;
type StylesArg =
  | string
  | CSSAttribute
  | StylesGenerator
  | Array<CSSAttribute | StylesGenerator>;
type StylesFn = (styles: StylesArg) => Component
type TagStyleGenerator = (props: {}) => number | string | undefined;
type TagFn = (
  tag: TemplateStringsArray,
  ...args: Array<string | TagStyleGenerator>,
) => Component;

// This way we can both use a regular function call with
// one argument or a tag function with multiple arguments
type StylingFn = StylesFn & TagFn;

With these types the example at the beginning wouldn't be allowed and the types would fit the implementation.

Let me know what you think.

100terres commented 2 years ago

We can track the progress here: https://github.com/solidjs/solid-styled-components/pull/38

100terres commented 1 year ago

related issue: https://github.com/solidjs/solid-styled-components/issues/15