Remove ThemeProps and add new theme state logic #47864
Conversation
vovakulikov
requested review from
philipp-spiess,
valerybugakov,
taras-yemets and
a team
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 39488a949104ae63acfa867c794743d82e0429d2...4f4c4539672d2e38e0d9e600bc939660abdc4174.
|
|
Codenotify: Notifying subscribers in OWNERS files for diff 39488a949104ae63acfa867c794743d82e0429d2...4f4c4539672d2e38e0d9e600bc939660abdc4174.
|
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits f9ca312 and 63f7afb or learn more. Open explanation
|
| }) | ||
| ) | ||
| } | ||
| // if (codeHost.isLightTheme) { |
There was a problem hiding this comment.
We probably don't need this anymore
|
I’m going to do a more detailed review tomorrow but I want to comment and say how excited I am about this change. I have wished for a |
|
❌ Problem: the label |
| autoFocus={true} | ||
| hideHelpButton={true} | ||
| /> | ||
| <ThemeContext.Provider value={{ themeSetting: !isDarkTheme ? ThemeSetting.Light : ThemeSetting.Dark }}> |
There was a problem hiding this comment.
I don't think we need a provider here if we have placed one already in client/jebtrains/webview/src/search/App.tsx
| export function useTheme(): useThemeApi { | ||
| const { themeSetting: contextThemeSetting, onThemeSettingChanging } = useContext(ThemeContext) | ||
|
|
||
| const systemTheme = useSyncExternalStore(subscribeToSystemTheme, getSystemThemeSnapshot) |
| useLayoutEffect(() => { | ||
| const isLightTheme = theme === Theme.Light | ||
|
|
||
| document.documentElement.classList.add('theme') | ||
| document.documentElement.classList.toggle('theme-light', isLightTheme) | ||
| document.documentElement.classList.toggle('theme-dark', !isLightTheme) | ||
| }, [theme]) |
There was a problem hiding this comment.
Should we extract this into a hook as well? Seems like we need it in a few places
| export const SiteAdminPingsPage: React.FunctionComponent<React.PropsWithChildren<Props>> = ({ isLightTheme }) => { | ||
| export const SiteAdminPingsPage: React.FunctionComponent<React.PropsWithChildren<Props>> = () => { |
There was a problem hiding this comment.
After removing most of the prop drilling, I imagine this is how most of the components will look. No props!
| isLightTheme={true} | ||
| onThemePreferenceChange={() => undefined} | ||
| showKeyboardShortcutsHelp={() => undefined} | ||
| themePreference={ThemePreference.Light} |
There was a problem hiding this comment.
This is huge, @vovakulikov! Thank you for looking into it 🔥
|
@valerybugakov @philipp-spiess Ideally, I want to generalise common core/infra state with context approach. For example, at the moment, we have some infrorations and services on the top level that many consumers within want to know about, like auth user, telemetry service, setting cascade, ...etc. And some of these global info stores were migrated from props (or partially migrated); for example, we have a special global store/hook for the experimental features. BUT i think having separate stores-hooks isn't a best idea here because you have to know about them to use it. Imagine if we shared it with one hook/context where we can fine all that we may need (and we had suggestions there) const { experimentalFeatures, theme, telemetryServices, ... } = useCore()Having one place to read core info implies that we have to create a separate package for it, and this is what stopped me from doing it here in this PR (it's just too big for one PR, and actually, there is a problem from decoupling temporary setting hook), If you have any ideas about it, please let me know. |
Hmmm I think I personally would prefer individual hooks for all of these because they don't really share logic and some of these (e.g. theme) might be updated where as others (e.g. telemetry stuff) are never changed which could cause unnecessary re-renderings when we have to subscribe to all context values. But yeah everything is better than the current prop-drilling 😅 |
|
@valerybugakov, it seems like there is a problem with vscode extension. I couldn't manage to run it locally due to problem with depencencie resolver, and I noticed that there are two deps that are not resolved but then |
|
Ok I think I got deps problem, so I manged to build vscode extension I think this is the ghost deps package problem, and this used to work before only because of how yarn and node deps resolution works (and how it doesn't work now with pnpm resolvers) But then when I try to debug this locally I see blank screen (no runtime errors found in the terminal), does anyone know here what it may be? Screen.Recording.2023-02-20.at.21.08.50.mov |
|
A few thoughts on jetbrains plugin
@philipp-spiess I remember you used to work with JB plugins, let me know if you have a minute to guide me through the standard process (maybe there is some catch with version that I'm missing) |
|
I managed to test code host integrations and browser extensions though, so it seems that after vscode and jb plugins we're good to go here |
vovakulikov
force-pushed
the
vk/improve-theme-state
branch
from
9a8e581 to
4f4c453
Compare
|
Regarding VS Code: No I don't remember seeing the blank screen before 😞 I also don't recall having had an issue with dependencies, @taras-yemets and me have made a change in vscode not too long ago! Regarding JetBrains: I can't get it to run myself either. It's crashing with this issue now 🤔
This seems unrelated to your change though. I wonder what goes on. I'll need more time to look into this I’m afraid 😭 |
|
@vovakulikov It does look like the build script for JetBrains is broken, yeah. I don't see the dist folder being created |
vovakulikov
force-pushed
the
vk/improve-theme-state
branch
from
4f4c453 to
e78b6fc
Compare
|
Here is a crazy idea (maybe not so bad actually if nobody has any things against it). What if we just forget about vscode and jetbrains plugins in this PR? I know that these two are not a priority at the moment (in fact, I haven't seen any big/or any changes in these plugins for a long time) So I propose to merge this PR and deal with possible regressions about theme in VSCode and JB plugin when we fix the vscode and jetbrains plugins). I would like to fix them, but at the moment, I don't have capacity even to dive deep there (maybe it's one liner fix, who knows). |
This sounds reasonable to me, it's very expensive to keep this branch open and we can't miss out on the benefits it brings. I would also add that a regression because of the themes is not very likely given that this is not a high risk change. WDYT @taras-yemets? |
There was a problem hiding this comment.
apparently, nobody uses this locally anymore, I think; this is why we didn't catch thing during pnpm migration
Eh, that's alarming. We should add this build to our CI pipeline! I created an issue for that: https://github.com/sourcegraph/sourcegraph/issues/48012
Agree with the proposal to merge this PR as is and create a follow-up issue to verify that the JetBrains extension works as expected later once we fix the build.
Inspired by Vova's #47864, this adds a new `useSettings()` hook to replace the existing `SettingsCascadeProps` prop drilling. I decided to _not_ replace all of the props drilling just yet to avoid a massive merge conflict when Vova's PR ships. I also really want to focus on cleaning up the app shell instead and I need this particularly for one use case: Cleaning up `globbing` which, as it turns out, is just a computed value from the setting instead. With this new settings API in place, I want to start trimming the shared props mess in a follow-up PR. This will then already remove the needs to pass the settings cascade around naturally and we will end up with a few smaller issues instead of a high change of breaking things. I've also marked the existing settings APIs (including access to the `SettingsCascade` object directly) as deprecated. `useSettings()` should be used when possible. I don't think we need to validate the settings in every caller manually.
vovakulikov
force-pushed
the
vk/improve-theme-state
branch
from
6f3f261 to
b6dff1f
Compare
vovakulikov
force-pushed
the
vk/improve-theme-state
branch
from
b6dff1f to
f9ca312
Compare
|
Ok, I'm about to merge this because I already got two big conflicts with main, so if something will show up because of this PR let me know |
Before
Before this PR we had theme state on the top level in the Layout.tsx, we had a hook that provided an object with public API for theme (to read or to set a new value), there were at least two problems
I think I don't need to dive deep in why prop drilling is a bad practice, but still, with prop drilling and nested components, we were updating the whole react tree when theme was changed, also passing these public Theme props required extending your component props with special type
ThemePropsand this becomes a mess quickly if you extend something else (like one component props extended other component props)Now
In this PR, we use more standard ways for global state distirubiotion - Context. In the new
useThemehook, we don't do any effects with CSS classes (consumer on the top level should handle it), so it's safe to call useTheme on any UI level. Privosly it wasn't safe, and because of this, we had this problem https://github.com/sourcegraph/sourcegraph/issues/47501Further steps
Ideally we shouldn't have any JS logic about theme besides CSS classes set on the document element. And all colors should work through CSS variables *custom-properties, but at the moment, we have at least two major places where we have to have JS check about theme.
Test plan
App preview:
Check out the client app preview documentation to learn more.