Change useExperimentalFeatures to use the new useSettings API
#48125
Conversation
This PR changes the `useExperimentalFeatures` hook to use the new `useSettings` API as discussed in #47979.
github-actions
bot
added
the
team/code-exploration
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits a594675 and 27d44ce or learn more. Open explanation
|
| </span> | ||
| </div> | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
This snapshot change happens because we enable codeMonitoringWebHooks is now enabled by default but the test was not covering the default settings properly. It only adds a CTA and is unrelated to the assertion though :)
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 27d44ce...a594675.
|
| // eslint-disable-next-line no-console | ||
| console.error( |
There was a problem hiding this comment.
The console.x methods are banned so we don't forget them in our PRs and to allow us to control the console output in place, where we potentially can send these logs/errors to other services like Sentry, Honeycomb, etc.
import { logger } from '@sourcegraph/common'
logger.error(...)There was a problem hiding this comment.
Cool. Is there a way to extend the error message here to explain this?
We have a lot of annoying eslint rules so I just try to make them go away quickly. In this case, I wasn't aware that this is for a good reason and the message didn't explain it either.
There was a problem hiding this comment.
We have a lot of annoying eslint rules so I just try to make them go away quickly.
True, we also have a bunch of duplicate rules because of multiple plugins we use. Let's raise this in #chat-frontend next time it happens so we can remove redundant rules and better document others.
Is there a way to extend the error message here to explain this?
There's no super straightforward way to do it AFAIK. We cannot use no-restricted-syntax because it already has a warning level. We cannot add a custom message to the no-console rule, according to the docs. We can probably create a custom rule and add our message there. Let me know if you see a better way. Here's the related section in our documentation that can be used in the custom error message.
| const { enableGoImportsSearchQueryTransform, applySuggestionsOnEnter } = useExperimentalFeatures(features => ({ | ||
| enableGoImportsSearchQueryTransform: features.enableGoImportsSearchQueryTransform, | ||
| applySuggestionsOnEnter: features.applySearchQuerySuggestionOnEnter ?? true, | ||
| })) |
There was a problem hiding this comment.
It looks like we don't need a selector function in most cases. Would it be good enough to return the experimental features object by default to avoid redundant field mapping?
| const { enableGoImportsSearchQueryTransform, applySuggestionsOnEnter } = useExperimentalFeatures(features => ({ | |
| enableGoImportsSearchQueryTransform: features.enableGoImportsSearchQueryTransform, | |
| applySuggestionsOnEnter: features.applySearchQuerySuggestionOnEnter ?? true, | |
| })) | |
| const { enableGoImportsSearchQueryTransform, applySuggestionsOnEnter = true } = useExperimentalFeatures() |
- As we discussed, there should be no performance issues caused by this update because the experimental features object is not mutable. We load it once and use it during the lifetime of the application. Updates are rare, and it's ok to re-rerender UI when they happen.
- It's important to discourage the renaming of things. With the selector API, it's easy to save keystrokes and name these settings as something else. Then find-and-replace doesn't work as well as it should, and it makes it harder to follow the logic in components.
There was a problem hiding this comment.
The object is mutated when the user settings change and in at least one feature that I’m aware of it is also mutated from inside the app code. I don't think the mental model of this being static is safe to apply here, but yeah it's rarely changed which makes it great for context.
My thinking was that we keep the API so we don't have to update all call sites and we can in the future use reactjs/rfcs#119 to make things faster again.
I can look into the change if you feel strongly though!
There was a problem hiding this comment.
Sounds good, let's keep it and also save time on refactoring 👍
| * This is a helper function to hide the fact that experimental feature flags | ||
| * are backed by a Zustand store | ||
| */ | ||
| export function getExperimentalFeatures(): SettingsExperimentalFeatures { |
There was a problem hiding this comment.
Nice! Is it twice a less code for the same functionality now?
|
@philipp-spiess, going ahead with merging this because I want to use this neat improvement in my work. 🙂 |
This PR changes the
useExperimentalFeatureshook to use the newuseSettingsAPI as discussed in #47979.Test plan
App preview:
Check out the client app preview documentation to learn more.