-
-
Notifications
You must be signed in to change notification settings - Fork 46
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: use unique keys for flow nodes #159
Conversation
// reset the form data completely | ||
setFlow(null) | ||
// Form submission was successful, show the message to the user! | ||
getFlow(data.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverting all of these changes from 85cc979 because they just masked the real issue.
key={ | ||
// input node | ||
"name" in node.attributes | ||
? node.attributes.name | ||
: // image node | ||
"src" in node.attributes | ||
? node.attributes.src | ||
: // anchor, text & script node | ||
"id" in node.attributes | ||
? node.attributes.id | ||
: k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the real fix. Previously a re-render after a flow was updated resulted in the same keys, although the input was vastly different. Because the mapping key here was the same, react did not properly re-render the input fields.
This is why index-based keys are always a bit tricky and should only be used when you are absolutely sure that the other props never change across re-renders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense!
key={ | ||
// input node | ||
"name" in node.attributes | ||
? node.attributes.name | ||
: // image node | ||
"src" in node.attributes | ||
? node.attributes.src | ||
: // anchor, text & script node | ||
"id" in node.attributes | ||
? node.attributes.id | ||
: k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense!
node={node} | ||
key={ | ||
// input node | ||
"name" in node.attributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we have TypeGuards for this? I believe we already use ory/integrations here, so might as well use the TypeGuards?
https://github.com/ory/integrations/blob/main/src/ui/index.ts#L87
https://github.com/ory/integrations/blob/main/src/ui/index.ts#L54
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call 👍
Closes #78 for real.