Refactor: no more direct imports of singletons, migrate all imports into React to use useSingletons hook instead
#9727
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
franknoirot
left a comment
franknoirot
left a comment
There was a problem hiding this comment.
Some initial self-review for us to discuss!
src/lib/singletons.ts
Outdated
| export const singletons = buildSingletons() | ||
| export const SingletonsContext = React.createContext(singletons) | ||
| export const useSingletons = () => React.useContext(SingletonsContext) |
There was a problem hiding this comment.
Important change number one: no more direct exports, only these 3 exports are available (plus one type def up top but who's counting).
src/index.tsx
Outdated
| iconTheme: { | ||
| primary: 'oklch(89% 0.16 143.4deg)', | ||
| secondary: 'oklch(48.62% 0.1654 142.5deg)', | ||
| <SingletonsContext.Provider value={singletons}> |
There was a problem hiding this comment.
Important change number two: providing the singletons as a single context object at the top of the component tree.
| return useMemo(() => { | ||
| return { value, onClick: onCodeNotificationClick, title: undefined } | ||
| }, [value]) | ||
| export const useDefaultAreaLibrary = () => { |
There was a problem hiding this comment.
Codex got a little refactor happy converting config objects like this on into hooks to make it possible to call useSingletons from here. I'm not sure how I feel about that pattern, but it does work the same at least in my user testing.
… instead of direct imports
… of direct imports
…ok instead of direct imports
…of direct imports
…k instead of direct imports
…ook instead of direct imports
…instead of direct imports
… instead of direct imports
…stead of direct imports
… hook instead of direct imports
…etons hook instead of direct imports
…Singletons hook instead of direct imports
…hook instead of direct imports
…hook instead of direct imports
…hook instead of direct imports
…k instead of direct imports
…tead of direct imports
…stead of direct imports
franknoirot
force-pushed
the
franknoirot/8885/inject-singletons-into-React
branch
from
9fac254 to
2b86b66
Compare
|
You know what, I actually think that it would be more prudent to wrap the singletons into a Class call |
Okay now I've done that. It's worth noting that what I've done so far does not pass the
Some things I don't like about this approach:
Blah, I'm gonna try to do away with that real quick. |
Truly completes #8885 by finishing the last step in the migration. Instead of directly importing singleton objects from
@src/lib/singletons.ts, we now provide them to the app in one place inindex.tsxon a React Context provider, then access them through a newuseSingletonshook.I whipped this up today by making the breaking changes in
singletons.tsandindex.tsx, doing one example of sometscfixes in the first commit, then prompting Codex to monkey-see-monkeybot-do with the loop. I cleaned up some of it's output but good lord it saved me a lot of keystrokes.Truth be told I'm not positive this is very helpful, so consider it more of a discussion PR. It seems like a smell to me that we can import from the singletons file all over React, but is it all that bad? And maybe a more fair question is this: is using a
useContexthook all that much better? Would the real improvement be to pass down the values needed wherever necessary, and not these often big singleton objects? Just some thoughts; I don't want this to get in the way of anyone else's work.