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

Inconsistency between styled's implementation and types #37

Closed
100terres opened this issue Oct 3, 2022 · 2 comments · Fixed by #38
Closed

Inconsistency between styled's implementation and types #37

100terres opened this issue Oct 3, 2022 · 2 comments · Fixed by #38

Comments

@100terres
Copy link
Contributor

100terres commented Oct 3, 2022

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:

let className = css.apply(
{ target: _ctx.target, o: append, p: withTheme, g: _ctx.g },
args
);

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
Copy link
Contributor Author

We can track the progress here: #38

@100terres
Copy link
Contributor Author

related issue: #15

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

Successfully merging a pull request may close this issue.

1 participant