Tweak ReactTypeOfWork order #13444
Tweak ReactTypeOfWork order #13444
Conversation
bvaughn
left a comment
bvaughn
left a comment
There was a problem hiding this comment.
I don't object to this change! but it seems like Sebastian might also want a chance to weigh in.
| export const ClassComponentLazy = 4; | ||
| export const FunctionalComponent = 0; | ||
| export const FunctionalComponentLazy = 1; | ||
| export const ClassComponent = 2; |
There was a problem hiding this comment.
This should have a big bold inline comment explaining the significance of 2
There was a problem hiding this comment.
I don't know why I added the word "bold" 🤡
There was a problem hiding this comment.
I think it'll be hard to miss any changes because they broke so much in internal tests (e.g. enzyme). We're also already very intentional about changing these constants.
If we merge this, it'll be just a temporary concession. We still have to "get our act together" later.
There was a problem hiding this comment.
I still think an inline comment might be helpful to avoid situations like the one that just happened (person A changing this, and person B discovering a few days later that it causes a lot of churn)
There was a problem hiding this comment.
I think the one that just happened seemed pretty intentional 😈 We should still fix this. If you feel strongly please feel free to send PR with a comment you'd find valuable. Maybe something like // Dependencies: *. But I'm also wary of making it seem like these are untouchable.
|
I'm going to merge this. If @sebmarkbage or @acdlite feel strongly against — let's chat tomorrow, but I think this shouldn't block the sync. If we want to change it, let's update the dependencies first, and then land an incompatible change into master. |
|
Also this change shouldn't make the jump table situation worse. |
|
Honestly half the reason to land this is to intentionally cause pain. The problem with these hacks is that there is not strong enough incentive to avoid them. So we keep landing more and more of them. The consequence is that we limit optimizations and make it harder to work in the core which is a cost for everyone downstream. In the end, we feel that we can’t land things upstream so all the incentive is aligned with making the core worse over time. This is a bad Nash equilibrium to be in. The way to solve such situations is to come up with artificial constraints to break the equivilibrium. In this case, causing friction until the incentive is to fix it downstream or more likely exposing a public api that works with both. Although a public api can’t predict new types of work (such as the lazy one). While this one is simple. We’re about to add and refactor several types of work. The lazy component is just one aspect of that. This will come up as a true blocker soon so we better unbreak now. IMO this is now the highest priority task. |
|
I'll follow up this week with a proper fix. I don't want to keep master in an unsyncable state though. |
* Update constants to match facebook/react#13444 * Update constants and support Lazy * Fix lint
* Update constants to match facebook/react#13444 * Update constants and support Lazy * Fix lint
|
For future reference: it shouldn't matter anymore and |
* Update constants to match facebook/react#13444 * Update constants and support Lazy * Fix lint
4 participants
I empathize with @sebmarkbage's concerns and I did the work of updating DevTools in facebook/react-devtools#1101.
But it turns RN's hot reloading transform also depends on this. It doesn't know the React version. We already have three forks of this transform (one forked by metro, and the original one has two versions that different tools depend on). That transform only depends on a single constant —
ClassComponentbeing2. Please don't make me re-release all of those. It's a huge timesink. It'll go away by itself when we have a better strategy for hot reloading, but for now let me keep this one constant.