-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Improve resolveConfig return type: merge themes #12272
Improve resolveConfig return type: merge themes #12272
Conversation
I'm used to using Gitlab which gives an option to squash commits when merging an MR. Is there no do the same in Gitlab? Please let me know if I should squash my commits on the branch itself. |
That's great, thank you @RobinMalfait ! I'm not particularly precious about getting the credit, but good to know—it was a bit weird that Github started showing all my commits as yours. I agree with your changes. I had forgotten prettier was to be used for formatting, I was expecting eslint to say something but it was falling over parsing typescript. And yes, the return type isn't perfect but it's much better. I had actually meant to write up the caveats. Maybe I'll add them into the PR description for posterity. |
* Generate types: do not intersect with Config theme type when generating DefaultTheme * Merge default theme in ResolvedConfig * UnwrapResolvables on theme.extend as well * Apply extend to overrides and default theme * Omit extend from DefaultTheme * Relax generic constraints, better generic variable names * Fall back to ThemeConfig if key not in DefaultTheme * Split out ThemeConfigCustomizable to avoid anys in ThemeConfigResolved * Allow custom theme properties * handle TypeScript error * apply prettier formatting * update changelog * change type name --------- Co-authored-by: Nikita Gaidakov <ngaidakov@podfather.com> Co-authored-by: Robin Malfait <malfait.robin@gmail.com>
This PR addresses #12264, adding some special handling of the
theme
key inResolvedConfig
to better reflect howtheme
andtheme.extend
are merged with the default theme values. Specifically:DefaultTheme
in the return valueconfig.theme
, it overrides the default theme valueconfig.theme.extend
, it's intersected with the resolved theme valueThe generated
DefaultTheme
type doesn't itself contain all possible theme config keys (those that are defined via a callback are omitted), so we fall back to values fromThemeConfig
, which are of a less concrete type (eg.KeyValuePair<string, string>
).Basically, I think I've implemented the "ridiculousness with conditional types" mentioned in the review for this PR
Incidental changes
A couple of small changes were necessary elsewhere in the codebase to facilitate this:
generate-types.js
: Do not intersect the generatedDefaultTheme
type withConfig['theme']
. We need to use the concrete value types in here to improve theResolvedConfig
type, and theConfig['theme']
types get in the way of this. As far as I can see, the only place whereDefaultTheme
is currently referenced is indefaultTheme.d.ts
where it is once again intersected withConfig['theme']
, so this change shouldn't make a difference to that.config.d.ts
: I exportedThemeConfig
to use inResolvedConfig
, and I also split out a separateThemeConfigCustomizable
interface which includes the final[key: string]: any
. Otherwise there was no way to avoidResolvedConfig
itself including a fallback[key: string]: any
, rendering any unknown property access valid in typescript's eye.Testing
Given that the codebase itself isn't written in typescript, I wasn't sure of a good way to test the changes. I resorted to creating a sample ts file defining a config, resolving it, and trying to access various keys on the output---using my IDE for typescript feedback.
Example test file
I'd gladly do more real-world testing or add tests to the codebase if an approach could be suggested.
This is my first PR to a large-scale OS project. I hope it's useful!