-
Notifications
You must be signed in to change notification settings - Fork 395
React Context happy fun times #1437
Comments
@annthurium I'm guessing that this is our path to dealing with our prop drilling problem. 🤔 I see three possible paths forward here:
What do you think? Were there other alternatives that you had in mind that we could pursue? |
I was thinking something along the lines of 1) because that seems the most amenable to incremental refactoring. I .. I don't love Redux because I feel like the functional language terminology it uses can be confusing, even though at its heart Redux is really just fancy pubsub. But just because I don't love it doesn't mean we shouldn't use it. What are the relative advantages of using Redux instead of the context api here? |
(1) was kind of how I'd been thinking about this before, certainly. Mostly I wanted to run the alternatives past you and see what thoughts you had 😄
Some of the more egregious drilled props in our codebase now are action methods - bound functions we pass through N layers of React to get to the actual button or whatever that triggers them. The easiest way I can think to get those to where they need to be is to lift them all up to the root of the component tree.. but then they need to be able to operate on whatever state they had access to at their previous homes in a controller somewhere, so the state would need to be lifted all the way up, too. And then we basically end up with Redux. Using Redux has the advantage that it's a pretty paved trail. A bunch of experience about state management in React apps is embedded in the library, and engineers who are not us would be responsible for maintenance. With that said... I don't particularly love it, either. I dislike having state and logic torn apart all across the codebase, and I'm not fond of introducing additional layers of indirection between everything rather than "passing props to components." I find it makes it harder to trace what's actually happening through the app. Caveat: I haven't actually used Redux in anger; I messed around in another project's source that was using... Flux I think?... which was a similar idea, and was thoroughly confused about everything the entire time.
See, I keep thinking I could roll us a less fancy pubsub to suit our needs... that might be the NIH speaking though 😆 I'm leaning toward at least starting with option (1). It should give us the most benefits the fastest, and give us an idea how much mess we'll be able to clean up so we can evaluate the relative merits of boiling the ocean later. |
So we pass a lot of props around.
github/lib/controllers/git-tab-controller.js
Lines 124 to 174 in fbaf0d4
React 16.3 introduced a fancy new Context API which doesn't even carry the "please don't use this" scare language that the old one had. We can likely take advantage of this to make our prop situation much more manageable, especially for accessing:
This isn't a trivial implementation, though. There are a lot of weird edge cases in the ways we handle event subscriptions, re-rendering timing, and so on that make this tricky to implement in a way that's a clear improvement. We would also have to be careful not to just re-implement Redux poorly.
The text was updated successfully, but these errors were encountered: