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

fix(recipes): applying recipes throws for unknown options #1381

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

christoph-fricke
Copy link

I recently started to move from class-variance-authority to vanilla-extract with recipes for the additional type safety. So far I am loving it apart from some nit-picks.

I am using both in React components where I find it quite handy to pass the props object to the recipe so it can apply all variants that are provided by the consumer. This works flawlessly with class-variance-authority. However, I noticed that the following code throws a runtime error "TypeError: Cannot read properties of undefined" in vanilla-extract when non-variant properties are applied as well. I would love to avoid having to de-structure and construct a variant options object all the time.

import { text, type TextVariants } from './text.css';

export interface TextProps extends TextVariants, ComponentPropsWithoutRef<"span"> {}

export function Text(props: TextProps) {
  return (
    <span {...props} className={text(props)}>
      {props.children}
    </span>
  );
}

This PR addresses the type error by ignoring unknown options when applying a recipe function. Therefore, recipes no longer throw a runtime error.

Copy link

changeset-bot bot commented Apr 12, 2024

🦋 Changeset detected

Latest commit: c39a65e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@vanilla-extract/recipes Patch
@fixtures/recipes Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kasper573
Copy link

kasper573 commented Apr 13, 2024

Don’t you need to destructure anyway to not pass on variant props to the underlying component, in this case the div?

i also use react and was frustrated by this same thing and realized the leaked variant props to the dom was triggering warnings and sometimes real problems. So i made a library to help create react components derived from recipes: https://www.npmjs.com/package/react-styled-factory

@christoph-fricke
Copy link
Author

@kasper573 I noticed that some props appear in the DOM when just spread onto the component, but I haven't seen any warnings, yet. However, I am quite early in the switch, so I might notice them down the line.

Nonetheless, I believe that the recipes function should not throw a runtime error when unknown props are passed in. So I think this PR is still relevant.

@aspirisen
Copy link
Contributor

Looks like I have created a PR with the same fix some time ago #1337

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 this pull request may close these issues.

3 participants