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: change the order of function overloading(createTheme) #1410

Conversation

pengooseDev
Copy link
Contributor

@pengooseDev pengooseDev commented May 10, 2024

PR Context

Fixes #1267

It seems to be a pretty valid way to deduce the parameters of the token by reordering the overloading of the function. However, the sideEffect(tradeOff) for the case where tokens are used as the first parameter should also be considered.
Based on the frequency of use of function signatures, I think the current changes can provide a better DX. :)
I'd appreciate it if you could give me some feedback on what you think about this.

// It seems be better to sort the function signatures by the frequency of user use. 🎯
export function createTheme<ThemeContract extends Contract>( 
  themeContract: ThemeContract,
  tokens: MapLeafNodes<ThemeContract, string>,
  debugId?: string,
): string;
export function createTheme<ThemeTokens extends Tokens>(
  tokens: ThemeTokens,
  debugId?: string,
): [className: string, vars: ThemeVars<ThemeTokens>];
export function createTheme(arg1: any, arg2?: any, arg3?: string): any {
  const themeClassName = generateIdentifier(
    typeof arg2 === 'object' ? arg3 : arg2,
  );

  registerClassName(themeClassName, getFileScope());

  const vars =
    typeof arg2 === 'object'
      ? createGlobalTheme(themeClassName, arg1, arg2)
      : createGlobalTheme(themeClassName, arg1);

  return vars ? [themeClassName, vars] : themeClassName;
}

Fixed

image

Copy link

changeset-bot bot commented May 10, 2024

🦋 Changeset detected

Latest commit: 99ba624

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

This PR includes changesets to release 27 packages
Name Type
@vanilla-extract/css Patch
@vanilla-extract/integration Patch
@vanilla-extract/rollup-plugin Patch
@fixtures/features Patch
@fixtures/layers Patch
@fixtures/low-level Patch
@fixtures/next-app-router Patch
@fixtures/next-pages-router Patch
@fixtures/recipes Patch
@fixtures/sprinkles Patch
@fixtures/template-string-paths Patch
@fixtures/themed Patch
@fixtures/thirdparty Patch
@fixtures/unused-modules Patch
@fixtures/thirdparty-dep Patch
@fixtures/thirdparty-dep-dep Patch
vanilla-extract-example-remix Patch
vanilla-extract-example-webpack-react Patch
@vanilla-extract-private/tests Patch
@vanilla-extract/esbuild-plugin-next Patch
@vanilla-extract/esbuild-plugin Patch
@vanilla-extract/jest-transform Patch
@vanilla-extract/parcel-transformer Patch
@vanilla-extract/vite-plugin Patch
@vanilla-extract/webpack-plugin Patch
@vanilla-extract-private/test-helpers Patch
@vanilla-extract/next-plugin 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

Copy link
Contributor

@askoufis askoufis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I don't really think frequency of use plays into this. It's really just exploiting the behaviour of function overloads to fix the big. It's not perfect, but I don't really think there's a better way to fix it.

@askoufis askoufis enabled auto-merge (squash) June 14, 2024 01:57
@askoufis askoufis merged commit 99e974b into vanilla-extract-css:master Jun 14, 2024
5 checks passed
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.

Typescript can't resolve which overload to use in createTheme() which makes it so we don't have autocomplete.
2 participants