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

Remove @emotion/styled dependency #1183

Closed
MMT-LD opened this issue Sep 12, 2020 · 8 comments · Fixed by #2043
Closed

Remove @emotion/styled dependency #1183

MMT-LD opened this issue Sep 12, 2020 · 8 comments · Fixed by #2043
Labels
affects-docs Changes affect documentation, but not only documentation enhancement New feature or request types

Comments

@MMT-LD
Copy link

MMT-LD commented Sep 12, 2020

Hey,

If i use Box and then i apply ref forwarding on a new component called button. It would seem that in typescript when using forward ref it seems that box is set to HTMLDivElement and if i supply HTMLButtonElement we get error that it needs to be a div element, i assume this is to do with the 'as' prop?

Just wandered what i'm doing wrong - i even tried the typings for ForwardRef and it still did not work. TS version 4.0.2

for example:

import * as React from 'react';
import { Box, BoxProps } from 'my-lib';

export interface IButtonProps extends BoxProps {
  label: string;
  variant?: 'primary' | 'secondary' | 'tertiary';
  children: React.ReactNode;
}

const Button = React.forwardRef<
  HTMLButtonElement,
  IButtonProps,
>(({ label = 'change me', variant = 'primary', ...rest }, ref) => {
  return (
    <Box
      as="button"
      ref={ref}
      variant={variant}
      aria-label={label}
      {...rest}
    />
  );
});

export { Button };
Type '((instance: HTMLButtonElement | null) => void) | MutableRefObject<HTMLButtonElement | null> | null' is not assignable to type 'string | ((instance: HTMLDivElement | null) => void) | RefObject<HTMLDivElement> | null | undefined'.
  Type '(instance: HTMLButtonElement | null) => void' is not assignable to type 'string | ((instance: HTMLDivElement | null) => void) | RefObject<HTMLDivElement> | null | undefined'.
    Type '(instance: HTMLButtonElement | null) => void' is not assignable to type '(instance: HTMLDivElement | null) => void'.
      Types of parameters 'instance' and 'instance' are incompatible.
        Type 'HTMLDivElement | null' is not assignable to type 'HTMLButtonElement | null'.
          Type 'HTMLDivElement' is missing the following properties from type 'HTMLButtonElement': disabled, form, formAction, formEnctype, and 13 more.ts(2322)
index.d.ts(143, 9): The expected type comes from property 'ref' which is declared here on type 'IntrinsicAttributes & ClassAttributes<HTMLDivElement> & HTMLAttributes<HTMLDivElement> & Pick<...> 

for now my fix is lame but its all i have got ;-)
ref={ref as any}

looking on emotion comments and it looks like the as prop has been thought about by different source - maybe a good reference?

https://github.com/kripod/react-polymorphic-box/blob/main/src/Box.tsx

Thanks in advance ;-)

@hasparus
Copy link
Member

Hey @MMT-LD 👋 Thanks for the issue!

This is the same problem as mentioned in #1124.
TLDR: Universal as prop and TypeScript are not good friends.

This is a known limitation. It's quite tricky to get as prop with field correctly working. on Box. Despite merging prop types with > the props of components passed in as prop is supported in Field, we stopped at the simplest implementation with Box.

You could work around this

  1. using Button exported from theme-ui and @theme-ui/components
  2. or by using Box.withComponent (it leaks from @emotion/styled, so it's not documented in Theme UI docs).
const BoxButButton = Box.withComponent("button")
  1. or using <button> with JSX pragma

Additional context: chakra-ui/chakra-ui#1582

@hasparus hasparus added the affects-docs Changes affect documentation, but not only documentation label Sep 12, 2020
@MMT-LD
Copy link
Author

MMT-LD commented Sep 13, 2020

@hasparus thanks for the workarounds. However, i think the JS community seems to be going towards typescript more and more (from looking at other libs and conversations) and i think if theme-ui could support the 'As' prop like Chakra, looks like they made significant progress over the last month, then that would be an amazing feature - otherwise its really only useful for non typescript users. Unless i'm missing the point of Box? or maybe we should be using/consuming the components differently?

Back to your points:

  1. Sure but i have to accept the defaults the lib provides - which i would rather avoid.
  2. This is what i have gone with but feels a bit hacky? - also i'm wandering if they may deprecate it like styled components are?
  3. We lose the m={} p={} props and can only use sx={} (is this more of the intention)?

Let me say that i really like this lib and i know how much work everybody puts into keeping a lib like this going 👍

@hasparus
Copy link
Member

hasparus commented Sep 13, 2020

could support the 'As' prop like Chakra, looks like they made significant progress over the last month, then that would be an amazing feature

I've spent a bunch of time fiddling with it, and I'm not convinced it's an amazing feature. TypeScript compilation times suffer drastically in large codebases using as prop (I even managed to get an OOM :D). It's a really cool gun to shoot oneself in the foot. I did it, and I'd strongly advise to use sx prop with jsx pragma in TypeScript.

Pretty soon, with React Automatic Runtime, you won't even need /** @jsx jsx */ comment.

Additional context:

https://blog.andrewbran.ch/polymorphic-react-components/

@MMT-LD
Copy link
Author

MMT-LD commented Sep 15, 2020

@hasparus just wanted to get clarification on withComponent from emotion so i asked the question.

Issue: emotion-js/emotion#2012

@lachlanjc
Copy link
Member

@hasparus Is #1274 enough of a fix for this, or should we do more to fix this?

@hasparus
Copy link
Member

@lachlanjc, I agree we should describe it in the docs, but we could move on this.

I agree with Andarist (emotion-js/emotion#2012 (comment)), that withComponent is quirky API. I think we can afford to deprecate it and remove it, and after we do, we could drop dependency on styled-system and @emotion/styled.

@lachlanjc
Copy link
Member

That sounds great, though will be a breaking change in requiring @emotion/styled to be installed separately (I use that on a few projects). I've never used withComponent on a Theme UI project (since using it on styled-components in the past was a nightmare personally & I didn't realize Theme UI even supported it). Plus, removing those two dependencies would noticeably bump down Theme UI package size, which I would be a big fan of.

@lachlanjc lachlanjc added the types label Dec 2, 2020
@lachlanjc lachlanjc changed the title Typescript forwarding ref issue Remove @emotion/styled dependency Dec 6, 2020
@lachlanjc lachlanjc added the enhancement New feature or request label Dec 6, 2020
@lachlanjc lachlanjc self-assigned this Dec 6, 2020
@lachlanjc lachlanjc removed their assignment Jan 4, 2021
@lachlanjc lachlanjc linked a pull request Oct 27, 2021 that will close this issue
@lachlanjc
Copy link
Member

Papertrail: we deprecated .withComponent some time ago & will be fully removing it soon.

@lachlanjc lachlanjc linked a pull request Dec 30, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-docs Changes affect documentation, but not only documentation enhancement New feature or request types
Projects
None yet
3 participants