frontend: Fix a circular dependency in KubeObject #2786
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
dosubot
bot
added
size:S
…link methods Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
…keKubeObject Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
Signed-off-by: Oleksandr Dubenko <oldubenko@microsoft.com>
sniok
force-pushed
the
kubeobject-circular-dependency-fix
branch
from
774c556 to
5f01d58
Compare
| path: '/storage/classes', | ||
| path: '/storageclasses', |
There was a problem hiding this comment.
What exactly are these fixing? I think changing a long standing URL may represent a breaking change.
There was a problem hiding this comment.
it's inconsistent with the rest of the paths, every other route is using /apiName/:namespace/:name
Comment on lines
+128
to
+132
| const cluster = this.cluster ?? getCluster(); | ||
| if (this.isNamespaced) { | ||
| return `/c/${cluster}/${this._class().apiName}/${params.namespace}/${params.name}`; | ||
| } | ||
| return `/c/${cluster}/${this._class().apiName}/${params.name}`; |
There was a problem hiding this comment.
Is replacing a function that produces a full URL by a repetition of the different URL parts a good idea? I'd say these changes make things harder to maintain and more error prone.
There was a problem hiding this comment.
yeah in the current form it will make it a bit worse.
but if we take this convention a step further and replace all the routes with:
{
details: {
path: '/:apiName/:?namespace/:name',
component: KubeObjectDetails,
},
list: {
path: '/:apiName/',
component: KubeObjectList
}
}I think it will make it less error prone in the long term
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes hot module reloading during development and reenables App.test.tsx
This is a fix for a problem I've been trying to fix for a long time. You might have noticed that during development if you're making any changes the whole page reloads. This is because Hot Module Reload is failing due to a circular dependency. Not every circular dependency causes this kind of failure.
This PR fixes circular dependency: KubeObject -> Router -> All Routes -> The whole app. By reimplementing getDetailsLink and getListLink methods without requiring router. Since routes follow the convention to use apiName, we can take a shortcut and avoid importing and assume the routes follow this convention. The only resources that weren't following this were Storage resources which were updated.
This unfortunately doesn't fix the storybook issue, so that workaround is still there