Land Lanes implementation in old fork #19108
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ada036b:
|
| // category: "Possible Errors", | ||
| // recommended: true, | ||
| // url: "https://eslint.org/docs/rules/no-extra-semi" | ||
| // }, |
There was a problem hiding this comment.
Oops, copypasta. Will delete.
Details of bundled changes.Comparing: 7f28234...ada036b react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 7f28234...ada036b react-dom
react-art
react-test-renderer
react-native-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
| @@ -2160,7 +2160,7 @@ export function attach( | |||
| const dependencies = | |||
| (fiber: any).dependencies || | |||
| (fiber: any).dependencies_old || | |||
| (fiber: any).dependencies_new; | |||
| (fiber: any).dependencies; | |||
There was a problem hiding this comment.
this statement doesn't look quite right
For each file in the new fork, copies the contents into the corresponding file of the old fork, replacing what was already there. In contrast to merge-fork, which performs a three-way merge.
First I ran `yarn replace-fork`. Then I ran `yarn lint` with autofix enabled. There's currently no way to do that from the command line (we should fix that), so I had to edit the lint script file.
Removes dead branches, removes prefixes from internal fields. Stuff like that.
acdlite
force-pushed
the
replace-old-fork
branch
from
4fc3d3b to
c7b73c2
Compare
DevTools tests only run against the old fork, which is why I didn't catch these earlier. There is one test that is still failing. I'm fairly certain it's related to the layout of the Suspense fiber: we no longer conditionally wrap the primary children. They are always wrapped in an extra fiber. Since this has been running in www for weeks without major issues, I'll defer fixing the remaining test to a follow up.
acdlite
force-pushed
the
replace-old-fork
branch
from
c7b73c2 to
ada036b
Compare
|
@bvaughn Some of the DevTools tests were broken. Only noticed now because DevTools tests only run against the old fork. The failures were related to the layout of the Suspense fiber, since the primary tree is now always wrapped in an extra fiber. I was able to fix most of them with a small change. There is one remaining test failure that I couldn't figure out. It's related to the same thing, but since I'm not familiar with that part of the DevTools code, I think if I tried to change anything else I'd end up breaking it in some other way. Since it's already been running in www for weeks, I figure it's OK to land as-is and fix it forward, once you're back from PTO and can give me some pointers. |
|
Ooh, we should get DevTools tests running against both reconcilers I guess. I wonder how hard that would be. Are we building the new reconciler into any artifacts (e.g. experimental? |
|
Is this the error you were seeing?
(This is the one I see when I uncomment those lines in I believe @gaearon wrote that particular test case (d81d74b) to cover how DevTools handles various Suspense state transitions. It looks like one of the transitions, specifically, causes this to fail. |
* Add autofix to cross-fork lint rule * replace-fork: Replaces old fork contents with new For each file in the new fork, copies the contents into the corresponding file of the old fork, replacing what was already there. In contrast to merge-fork, which performs a three-way merge. * Replace old fork contents with new fork First I ran `yarn replace-fork`. Then I ran `yarn lint` with autofix enabled. There's currently no way to do that from the command line (we should fix that), so I had to edit the lint script file. * Manual fix-ups Removes dead branches, removes prefixes from internal fields. Stuff like that. * Fix DevTools tests DevTools tests only run against the old fork, which is why I didn't catch these earlier. There is one test that is still failing. I'm fairly certain it's related to the layout of the Suspense fiber: we no longer conditionally wrap the primary children. They are always wrapped in an extra fiber. Since this has been running in www for weeks without major issues, I'll defer fixing the remaining test to a follow up.
|
I'm still a little baffled by this error. Through a bit of bisecting, I was able to suppress the bizarre The thing that makes this weird is that the error is being thrown by I must be forgetting how one of these pieces connects. Edit 1 Looks like there's an extra call to (ReactDOM) Edit 2 Ah, that's because the DevTools test is calling Edit 3 I assume the problem is that DevTools has a bad assumption about the state of the tree b'c of the change in the extra wrapper Fiber. Reverting the changes made to Edit 4 Looks like the logic for |
|
Okay. Coming back at this today with a fresh perspective. The test has a component called const Never = () => {
throw new Promise(() => {});
};I think what's happening is this:
Step 3 is different behavior between the old and new reconciler implementations. (The old reconciler doe not try to commit a deletion for So why does the new one? Stepping through the code, the force suspend doesn't actually tag The failing test case can be reduced to these three steps: act(() =>
ReactDOM.render(
<Wrapper
suspendParent={false}
suspendFirst={true}
suspendSecond={true}
/>,
container
)
);
// This update deletes the Never component rendered by the previous commit:
act(() =>
ReactDOM.render(
<Wrapper
suspendParent={false}
suspendFirst={false}
suspendSecond={false}
/>,
container
)
);
// This update incorrectly tries to commit a second deletion for Never:
const rendererID = getRendererID();
act(() =>
agent.overrideSuspense({
id: store.getElementIDAtIndex(4),
rendererID,
forceFallback: true,
})
);Edit I think the problem is that the call to |
|
While debugging this, I also noticed that running tests with the new |
Ooh I did remove the check for |
DevTools has a feature to force a Suspense boundary to show a fallback. This feature causes us to skip the first render pass (where we render the primary children) and go straight to rendering the fallback. There's a Legacy Mode-only codepath that failed to take this scenario into account, instead assuming that whenever a fallback is being rendered, it was preceded by an attempt to render the primary children. SuspenseList can also cause us to skip the first pass, but the revelant branch is Legacy Mode-only, and SuspenseList is not supported in Legacy Mode. Fixes a test that I had temporarily disabled when upstreaming the Lanes implementation in facebook#19108.
DevTools has a feature to force a Suspense boundary to show a fallback. This feature causes us to skip the first render pass (where we render the primary children) and go straight to rendering the fallback. There's a Legacy Mode-only codepath that failed to take this scenario into account, instead assuming that whenever a fallback is being rendered, it was preceded by an attempt to render the primary children. SuspenseList can also cause us to skip the first pass, but the relevant branch is Legacy Mode-only, and SuspenseList is not supported in Legacy Mode. Fixes a test that I had temporarily disabled when upstreaming the Lanes implementation in facebook#19108.
DevTools has a feature to force a Suspense boundary to show a fallback. This feature causes us to skip the first render pass (where we render the primary children) and go straight to rendering the fallback. There's a Legacy Mode-only codepath that failed to take this scenario into account, instead assuming that whenever a fallback is being rendered, it was preceded by an attempt to render the primary children. SuspenseList can also cause us to skip the first pass, but the relevant branch is Legacy Mode-only, and SuspenseList is not supported in Legacy Mode. Fixes a test that I had temporarily disabled when upstreaming the Lanes implementation in facebook#19108.
DevTools has a feature to force a Suspense boundary to show a fallback. This feature causes us to skip the first render pass (where we render the primary children) and go straight to rendering the fallback. There's a Legacy Mode-only codepath that failed to take this scenario into account, instead assuming that whenever a fallback is being rendered, it was preceded by an attempt to render the primary children. SuspenseList can also cause us to skip the first pass, but the relevant branch is Legacy Mode-only, and SuspenseList is not supported in Legacy Mode. Fixes a test that I had temporarily disabled when upstreaming the Lanes implementation in #19108.
|
This ended up being the crux of the issue:
See #19164 for the fix. Thanks for narrowing that down! |
The |
DevTools has a feature to force a Suspense boundary to show a fallback. This feature causes us to skip the first render pass (where we render the primary children) and go straight to rendering the fallback. There's a Legacy Mode-only codepath that failed to take this scenario into account, instead assuming that whenever a fallback is being rendered, it was preceded by an attempt to render the primary children. SuspenseList can also cause us to skip the first pass, but the relevant branch is Legacy Mode-only, and SuspenseList is not supported in Legacy Mode. Fixes a test that I had temporarily disabled when upstreaming the Lanes implementation in facebook#19108.
Replaces the old reconciler fork with the contents of the new reconciler fork.
There are still separate "old" and "new" forks, but they are now identical. We will continue to use the fork infra to land risky changes and refactors.
Most of the changes were automated using a new command,
yarn replace-fork. I put the manual changes (removing dead branches, cleaning up tests, etc) into a separate commit.