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

fix: "Node not found" error from 'getNode()' #4608

Merged
merged 4 commits into from
Jun 8, 2023

Conversation

zqqcee
Copy link
Contributor

@zqqcee zqqcee commented Jun 8, 2023

Checklist

  • [ x ] npm test passes
  • [ x ] commit message follows commit guidelines

Description of change

undo delete workflow

To fix the bug Node not found, I've changed packages/g6/src/runtime/controller/item.ts

image

old version:

const innerModel = graphCore.getNode(id);
const relatedEdgeInnerModels = graphCore.getRelatedEdges(id);

new version:

// 'id' variable is always string in here, but one in user data is number, possibly.        
let innerModel
try {
  innerModel = graphCore.getNode(id);
} catch (e) {
  innerModel = graphCore.getNode(Number(id))
}

let relatedEdgeInnerModels;
try {
  relatedEdgeInnerModels = graphCore.getRelatedEdges(id);
} catch (error) {
  relatedEdgeInnerModels = graphCore.getRelatedEdges(Number(id));
}

Why this change

For the bug Node not found mentioned before

An error is throwed from this function 'getNode()'; msg: "Node not found for id: 1"
After testing, I found that 'id' variable is always string after Object.keys(update).forEach((id)=>{...}), but one in user data is number, possibly.
I tried adding a try-catch block to fix this bug, Now:

  1. v5 already supports users to use both string and number types of id
  2. Meanwhile, it also supports users to pass in mixed type ids, like:
const data = {
        nodes: [
            {
                id: 1,
                data: {
                    x: 100,
                    y: 100,
                }
            },
            {
                id: '2',
                data: {
                    x: 200,
                    y: 100,
                }
            },
        ],
    };

This PR is modified from #4599

  • adding try-catch to solves the bug : "Node not found for id"
    The test demo located in here: packages/g6/tests/intergration/bugReproduce.ts

@zqqcee
Copy link
Contributor Author

zqqcee commented Jun 8, 2023

I'm so sorry 😭! Because of the personal token, the file with workflow cannot be pushed, so I deleted the workflow. But now I have create a new personal token and undone the delete, here is my recreated PR.

@Yanyan-Wang
Copy link
Contributor

Great! Thanks for your pull request. Don't hesitate to ask me if you have any questions.

@Yanyan-Wang Yanyan-Wang merged commit 65c599e into antvis:v5 Jun 8, 2023
2 checks passed
@zqqcee
Copy link
Contributor Author

zqqcee commented Jun 8, 2023

I‘ll close the issue #4596 , thank you~!

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

Successfully merging this pull request may close these issues.

None yet

2 participants