-
Notifications
You must be signed in to change notification settings - Fork 113
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 FE when tag
name is undefined
#2146
Conversation
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Kacper confirmed that this PR resolves the main issue i.e it doesn't break Kedro-viz; the outputs of pipelines are not entirely correct. and when he gets time he will create some example pipelines that we can use to reproduce and fix this bug. FYI @astrojuanlu |
Signed-off-by: rashidakanchwala <[email protected]>
src/utils/index.js
Outdated
@@ -120,7 +120,7 @@ export const stripNamespace = (str) => { | |||
* @param {String} str The string to check | |||
* @returns {String} The string with or without replaced values | |||
*/ | |||
export const prettifyName = (str) => { | |||
export const prettifyName = (str = '') => { |
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.
@rashidakanchwala
The default value will only handle undefined, it will not handle cases of null
. Though prettifyName will never be called with null, I would still suggest using an explicit check like below to handle all weird cases -
export const prettifyName = (str) => {
if (!str) {
return '';
}
...
Signed-off-by: rashidakanchwala <[email protected]>
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.
Thanks @rashidakanchwala!
Description
Partly resolves #2106
Development notes
In the user's pipeline, one of the nodes was tagged as
validation
. While this tag was mentioned in the{ node: { tags: [] } }
response, it wasn't somehow mentioned in the{ tags: {} }
response which caused the flowchart to break. We were unable to reproduce this issue on the backend, which would have been the optimal place to address it.As a workaround, we fixed this on the frontend. When a tag is not defined correctly in the backend, the frontend cannot link the tag to the corresponding tag name. Since the tag name and ID are the same, we added a fallback. If the tag name is undefined (due to an error we still need to investigate), we use the tag ID itself as a fallback. This prevents Kedro-Viz from breaking and ensures the flowchart runs correctly.
QA notes
To test this fix using the demo project:
Run
kedro viz --save-file=test
. This will create a test folder with an api folder inside.Move the
api
folder into thekedro-viz/public
directory.Remove 'company' dict from
tags:
inapi/main
folderRun
npm run start:dev
.Before this PR, you will see the app breaking with the error message the user encountered. After applying the PR, the issue will be fixed.
Checklist
RELEASE.md
file