Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error: "Cannot add node "1" because a node with that id is already in the Store." #21436

Closed
dharapx opened this issue May 5, 2021 · 34 comments · Fixed by #21523
Closed

Error: "Cannot add node "1" because a node with that id is already in the Store." #21436

dharapx opened this issue May 5, 2021 · 34 comments · Fixed by #21523

Comments

@dharapx
Copy link

dharapx commented May 5, 2021

Which website or app were you using when the bug happened?

Please provide a link to the URL of the website (if it is public), a CodeSandbox (https://codesandbox.io/s/new) example that reproduces the bug, or a project on GitHub that we can checkout and run locally.

What were you doing on the website or app when the bug happened?

If possible, please describe how to reproduce this bug on the website or app mentioned above:

Generated information

DevTools version: 4.13.1-93782cfed2

Call stack:
(none)

Component stack:
(none)

GitHub URL search query:
https://api.github.com/search/issues?q=Cannot%20add%20node%20%20because%20a%20node%20with%20that%20id%20is%20already%20in%20the%20Store.%20in%3Atitle%20is%3Aissue%20is%3Aopen%20is%3Apublic%20label%3A%22Component%3A%20Developer%20Tools%22%20repo%3Afacebook%2Freact

@bvaughn
Copy link
Contributor

bvaughn commented May 5, 2021

@dharapx Sorry you hit this problem

Unfortunately though, it doesn't look like this bug report has enough info for one of us to reproduce it.

Please provide a CodeSandbox (https://codesandbox.io/s/new), a link to a repository on GitHub, or provide a minimal code example that reproduces the problem. Screenshots or videos can also be helpful if they help provide context on how to repro the bug.

Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

@ZackeryBayle
Copy link

Project: https://github.com/ZackeryBayle/TrollyReactJS
Back end: https://github.com/ZackeryBayle/TrollyCRM

While working on the project doing some code splitting.

Update existing issue: Error: "Cannot add node "1" because a node with that id is already in the Store." Uncaught Error: Cannot add node "1" because a node with that id is already in the Store.
I am pretty new so please forgive the crazy mess you are about to witness.

@bvaughn
Copy link
Contributor

bvaughn commented May 6, 2021

Hey @ZackeryBayle! This is a great help.

While working on the project doing some code splitting.

Can you elaborate a little more on what I'd need to do to reproduce the error myself beyond running these two projects locally?

@ZackeryBayle
Copy link

@bvaughn - I was working on just rendering some props on the home screen after splitting my form code into its own component.
On render throws error
Page still renders

I hope that helps, let me know if there is anything else I can provide.

@ZackeryBayle
Copy link

I have simply removed everything from the rendering page (home.js) and the error still happens. The only thing being rendered is my navBar and footer. Nothing has changed on these components for weeks. I haven't done any updates to dependence.

image

@bvaughn
Copy link
Contributor

bvaughn commented May 6, 2021

Oh that's excellent. Can you push a branch and point me at it so I can check it out and repro?

I got stuck having to install mongodb this morning to run the backend and then I had meetings.

@ZackeryBayle
Copy link

Here is the new branch https://github.com/ZackeryBayle/TrollyReactJS/tree/staging-issues

@bvaughn
Copy link
Contributor

bvaughn commented May 6, 2021

Thanks!

@bvaughn
Copy link
Contributor

bvaughn commented May 6, 2021

@ZackeryBayle So you just load the page and see the error?

I can't repro that. (Now that Mongo is installed.)

Do you have any component filters or anything on in your devtools by chance? Would I need any dummy data in my mongodb to repro this maybe?

@ZackeryBayle
Copy link

correct Just loading the page produces that error. Nothing about the mongo should any effect on this atm because all the components have been removed from the pages/home.js

I don't know what a component filter is sorry I'm really new at this. This is actually my capstone project for bootcamp.

@bvaughn
Copy link
Contributor

bvaughn commented May 6, 2021

Ah ok that's interesting. I wonder why I'm not able to reproduce the bug using the same branch/app.

Any chance you'd be up for a quick Zoom call tomorrow to look at it together? No problem if you're not, but if you are– shoot me an email (brian.david.vaughn at gmail) or a DM on twitter (@ brian_d_vaughn)

@ZackeryBayle
Copy link

Update: I have restarted my development server this morning and now am not getting the error. I will load in components one at a time to see if any of them will recreate this error and report back when down.

@bvaughn
Copy link
Contributor

bvaughn commented May 7, 2021

Thank you for the update, @ZackeryBayle

@ZackeryBayle
Copy link

Okay all components are loaded back in and rendering... no more error. I'm at a loss on this one.

Only thing I can say is I was trying to figure out how to raise state so I could update a a parent state from a child when this happened. Let me know if there is anything else I can do to provide any more info or help.

@bvaughn
Copy link
Contributor

bvaughn commented May 7, 2021

Aw, rats. If you're able to repro again, let me know. Unfortunately, I don't think there's much I can do until I get a repro case.

@yrambler2001
Copy link

yrambler2001 commented May 7, 2021

image
happens randomly for me too

@bvaughn
Copy link
Contributor

bvaughn commented May 7, 2021

@yrambler2001 Need a repro case. Can you share one?

@yrambler2001
Copy link

@bvaughn Unfortunately, it happens too randomly for me, page refresh with dev tools refresh fixes this for me.

@bvaughn
Copy link
Contributor

bvaughn commented May 8, 2021

Understood. If you're able to narrow it down, even to something that happens once and a while, repro steps would be helpful.

@eps1lon
Copy link
Collaborator

eps1lon commented May 8, 2021

Repro: https://github.com/eps1lon/react-devtools-repro-no-matching-node/tree/main
React devtools version: 4.13.2 (e468072)
Steps included in repro.

react-devtools-repro-no-matching-node.mp4

@bvaughn
Copy link
Contributor

bvaughn commented May 9, 2021

Thanks for the repro, @eps1lon!


What seems to be happening is that Refresh sets a _debugNeedsRemount flag, which clones the old Fiber and splices it into the tree in-place. (This breaks some assumptions on the DevTools side FWIW.)

Then simultaneously, the component logs a warning during render, which the backend sees– but when it tries to find the Fiber in its internal map, it sees that it's a new one and creates a new ID for it.

Then during the subsequent render, it tries to unmount the old Fiber, but with the new ID.

I'm not sure yet exactly what the right way to fix this is. Errors or warnings logged during initial render that belong to untracked (or not-yet-tracked) Fibers are important. (Most warnings probably happen during initial render.) Need to give this some more thought.

@bvaughn
Copy link
Contributor

bvaughn commented May 12, 2021

I think I've captured the bug in a unit test:
https://github.com/bvaughn/react/blob/d49ba6b598522e4a5c0d93e215d7e3c7c51fef2c/packages/react-devtools-shared/src/__tests__/FastRefreshDevToolsIntegration-test.js#L164-L254

If I remove the render-phase console.warn the test will pass.

The overall pattern of the failed test is:

Mount

  1. A warning is logged for Component (id:1)
  2. recordMount() for Component (id:1)

Patch 1

  1. A warning is logged for Component (now id:4 because it was cloned by Fast Refresh)
  2. updateFiberRecursively() for Component (id:4)

Patch 2

  1. A warning is logged for Component (now id:7 because it was cloned by Fast Refresh)
  2. recordUnmount() for Component (id:4) -> This throws!
  3. recordMount() for Component (id:7)
  4. recordResetChildren() for Component (id:7) 3:null (2)

Unfortunately the unmount for id 4 causes the Store to throw, because the component was mounted with an id of 1.

Edit This is a bit embarrassing, but I think the test above only catches the bug if __DEBUG__ is enabled because logging the Fiber (when the warning/error is caught) causes a new ID to be generated for it. The bug still happens in a CRA app with Refresh enabled, but I'm having trouble reproducing it in a test case. That said, I can see that we're leaking fibers (even though the test doesn't throw).

@bvaughn
Copy link
Contributor

bvaughn commented May 12, 2021

I'm not sure of the best way to handle this. DevTools can read the _debugNeedsRemount field during unmount to detect what's happened, but at this point– how does it get from id 4 back to id 1? I think I'd have to add a lot of metadata tracking to be able to map from type-to-id.

I think the best approach might be to add a hook to React to inform DevTools of when a forced clone+remount has happened.

@bvaughn
Copy link
Contributor

bvaughn commented May 13, 2021

This test captures the bug:

withErrorsOrWarningsIgnored(["Expected warning during render"], () => {
  render(`
      const {useState} = React;

      export default function Component() {
        const [state, setState] = useState(1);

        console.warn("Expected warning during render");

        return <div />;
      }
    `);
});
expect(store).toMatchInlineSnapshot(`
      ✕ 0, ⚠ 1
      [root]
          <Component> ⚠
    `);

let element = container.firstChild;

withErrorsOrWarningsIgnored(["Expected warning during render"], () => {
  patch(`
      const {useState} = React;

      export default function Component() {
        const [state, setState] = useState(1);

        console.warn("Expected warning during render");

        return <div id="one" />;
      }
    `);
});
expect(store).toMatchInlineSnapshot(`
      ✕ 0, ⚠ 2
      [root]
          <Component> ⚠
    `);

// State is preserved; this verifies that Fast Refresh is wired up.
expect(container.firstChild).toBe(element);
element = container.firstChild;

withErrorsOrWarningsIgnored(["Expected warning during render"], () => {
  patch(`
      const {useEffect, useState} = React;

      export default function Component() {
        const [state, setState] = useState(3);
        useEffect(() => {});

        console.warn("Expected warning during render");

        return <div id="one" />;
      }
    `);
});
expect(store).toMatchInlineSnapshot(`
      ✕ 0, ⚠ 3
      [root]
          <Component> ⚠
          <Component> ⚠
    `);

In the third snapshot, there should not be a duplicate <Component>.

@bvaughn
Copy link
Contributor

bvaughn commented May 13, 2021

I think #21506 might be one way to fix this issue.

@bvaughn
Copy link
Contributor

bvaughn commented May 18, 2021

@eps1lon, @ZackeryBayle Would either of you be willing to try this build and let me know if it fixes your issue?

ReactDevTools.zip

@bvaughn
Copy link
Contributor

bvaughn commented May 18, 2021

#21516 might fix this problem but without a repro I can't be sure. I'm going to close this issue for now though and we can re-open (with a repro) if that's not the case.

@bvaughn bvaughn closed this as completed May 18, 2021
@eps1lon
Copy link
Collaborator

eps1lon commented May 18, 2021

@eps1lon, @ZackeryBayle Would either of you be willing to try this build and let me know if it fixes your issue?

ReactDevTools.zip

@bvaughn I can still reproduce my repro (#21436 (comment)) with
the devtools version in your zip (4.13.2-723f44a088).

@bvaughn bvaughn reopened this May 18, 2021
@bvaughn
Copy link
Contributor

bvaughn commented May 18, 2021

That's so interesting. I tested a variation of your repro (without the error logged inside the effect). My change fixes that repro in the browser, or both cases in a unit test, but...sure enough, I can see the error you're referring to in the Store now too :(

My bad! Let me dig further.

@bvaughn
Copy link
Contributor

bvaughn commented May 18, 2021

I see a difference, but I'm not sure I understand it. Based on logging...

// DevTools mounts Component version X with ID 5
mountFiberRecursively() Component (5) 
recordMount() Component (5) 
recordResetChildren() Component (5) 

// DevTools untracks Component version X (...) with ID 5 because
// an error was previous logged during render (above) and
// this schedules a check after a small delay to clear up fibers
// that rendered but never mounted.
// In this case, we incorrectly identify this Fiber as unmounted
// because it's been disonnected from the tree by Fast Refresh.
untrackFiberID() Component (5) 

// DevTools sees an error logged for (old) Component version X
// in a passive effect that gets flushed before the remount render
// and generates a new ID 6 for it
getOrGenerateFiberID() Component (6) Generated a new UID
onErrorOrWarning Component (6)  error: "effect"

// DevTools sees an error logged for Component version X+1
// during render and generates a new ID 7 for it
getOrGenerateFiberID() 2:Component (7) Generated a new UID
onErrorOrWarning 2:Component (7)  warn: "render"

// DevTools tries to unmount Component version X with ID 6
// (but it should be unmounting version X with ID 5)
recordUnmount() Component (6)  

I'm not sure what needs to change to fix this.

@bvaughn
Copy link
Contributor

bvaughn commented May 18, 2021

@eps1lon Can you try this build? 😬

ReactDevTools.zip

@bvaughn
Copy link
Contributor

bvaughn commented May 19, 2021

Should be fixed by #21523.

If you'd like to verify, download this build:
ReactDevTools.zip

And follow these steps to install it:
https://user-images.githubusercontent.com/29597/118749019-b6cda600-b82a-11eb-9449-ef56f41eced5.mp4

@eps1lon
Copy link
Collaborator

eps1lon commented May 19, 2021

@eps1lon Can you try this build? 😬

ReactDevTools.zip

Still the same issue with DevTools version 4.13.3-0f72f7361f, Chrome Version 90.0.4430.212 (Official Build) (64-bit) on Windows 10 and react-dom 17.0.2.

Let me check if this also reproduces on Linux.

@bvaughn
Copy link
Contributor

bvaughn commented May 19, 2021

@eps1lon Wait, you still see this with the repro above? Are you sure you're running the latest unpacked extension? I'm able to repro it 100% of the time without the fix and 0% with the fix.

Maybe we could VC and look at it together if you're still reproing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants