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

getting theme values, with type-safety! #1128

Closed
wants to merge 5 commits into from
Closed

getting theme values, with type-safety! #1128

wants to merge 5 commits into from

Conversation

smockle
Copy link
Member

@smockle smockle commented Mar 18, 2021

get is a function that takes a dot-delimited path and returns a theme value from the theme object.

Previously, non-existent paths could be passed to get, resulting in misapplied styles that could only be caught at runtime (i.e. when previewing a page in the browser).

This PR updates get to be type-safe:

  • Red squiggles appear (compile-time errors) when a non-existent path is used
  • Nested keys are suggested when . is typed
  • Theme values can be previewed on hover

Screenshots

Path autocomplete
autocomplete

Theme value previews
preview-values

Before type-safety
before

After type-safety
type-safe

@changeset-bot
Copy link

changeset-bot bot commented Mar 18, 2021

⚠️ No Changeset found

Latest commit: 1c32c7a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link

vercel bot commented Mar 18, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/E3VRkqkxYFnvXJTt8brQ59HgtYPo
✅ Preview: Failed

[Deployment for 1c32c7a failed]

@smockle
Copy link
Member Author

smockle commented Mar 19, 2021

Vercel deployments are failing, but I don’t have access to see the reason why. cc @colebemis @emplums

@colebemis
Copy link
Contributor

@smockle This is awesome! 🎉

Looks the bundle size increased pretty substantially though (+90%).

Is there a way to get this to work with preval? We were using preval originally to avoid adding dependencies (e.g. polished) that are only need to build the theme object.

@smockle
Copy link
Member Author

smockle commented Mar 29, 2021

From @colebemis in #1128 (comment):

Looks the bundle size increased pretty substantially though (+90%).

Is there a way to get this to work with preval? We were using preval originally to avoid adding dependencies (e.g. polished) that are only need to build the theme object.

Copying this from one of my recent stand-up messages, since it’ll be a while before I can return to this PR:

I have several ideas for addressing @colebemis’ comment on the type-safe get PR re: bundle size:

  • Adding a second tsconfig.json + build step to compile the theme separately and then consume the result JS + .d.ts in the main project.
  • Eliminating unneeded dependencies (e.g. lodash’s isObject is 2 lines of code) to reduce transitive dependencies.
  • Or, restoring the JS + preval pipeline but adding a handwritten .d.ts (least preferred option, because I think it’d get out-of-sync).
  • Or, peruse Let's discuss TypeScript support. kentcdodds/babel-plugin-macros#94 “Let’s discuss TypeScript support” again for recommended solutions.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

@github-actions github-actions bot added the Stale label Sep 2, 2021
@github-actions github-actions bot closed this Sep 9, 2021
@github-actions github-actions bot deleted the typesafe-get branch September 9, 2021 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants