Add cluster in url - Newest cleaned up code #7
Conversation
| import { LAST_CLUSTER_USER_SETTINGS_KEY } from '@console/shared/src/constants'; | ||
| import { useUserSettings } from '@console/shared/src/hooks/useUserSettings'; | ||
| import store from '../../../../../public/redux'; |
There was a problem hiding this comment.
Add the store isn't the best option, but there were weird bugs when using useActiveNamespace. The bug is most notable when going to the pods list page for all projects and then clicking on a pod. Instead of going to the details page, the user was routed back to the list page.
There was a problem hiding this comment.
Do you see the same behavior is you use useSelector directly?
return useSelector(({ UI }) => UI.get('activeNamespace'));
There was a problem hiding this comment.
Good question. I thought about that last night. I'll check it out.
There was a problem hiding this comment.
First changed the path to the store to import store from '@console/internal/redux'; so at least it isn't relative.
So neither the useSelector nor the activeNamespace from useActiveNamespace worked. Here is what I think is going on. We need to know the current Namespace inside an useEffect hook, but since useSelector and useActiveNamespace are hooks and React gets grumppy when trying to put a hook in hook, I need to put them outside of the useEffect block. I'm thinking that when I do that the namespace from the hook is a step behind when calling the useEffect.
So not sure what to do.
| @@ -10,6 +10,7 @@ import { | |||
| ALL_NAMESPACES_KEY, | |||
| LAST_NAMESPACE_NAME_LOCAL_STORAGE_KEY, | |||
| } from '@console/shared/src/constants'; | |||
| // import { multiClusterRoutePrefixes } from '@console/app/src/components/detect-cluster/cluster' | |||
There was a problem hiding this comment.
Using the import caused most of the tests to fail. Unsure of cause, so the prefixes are duplicated in the code below.
There was a problem hiding this comment.
I tested this again. When you import the multiClusterRoutePrefixes it causes this test suite to fail:
packages/console-shared/src/hooks/__tests__/useUserSettings.spec.ts
Each test fails with this message:
useUserSettings › should return default value for an empty configmap after switching from loading to loaded
TypeError: user_settings_1.seralizeData is not a function
Seems kind of crazy pants to me, but I tested it numerous times and it fails without fail (hahaha) when using the import.
There was a problem hiding this comment.
Stack trace for failed test:
at invokePassiveEffectCreate (node_modules/react-dom/cjs/react-dom.development.js:23487:20)
at HTMLUnknownElement.callCallback (node_modules/react-dom/cjs/react-dom.development.js:3945:14)
at Object.invokeGuardedCallbackDev (node_modules/react-dom/cjs/react-dom.development.js:3994:16)
at invokeGuardedCallback (node_modules/react-dom/cjs/react-dom.development.js:4056:31)
at flushPassiveEffectsImpl (node_modules/react-dom/cjs/react-dom.development.js:23574:9)
at unstable_runWithPriority (node_modules/scheduler/cjs/scheduler.development.js:646:12)
at runWithPriority$1 (node_modules/react-dom/cjs/react-dom.development.js:11276:10)
at flushPassiveEffects (node_modules/react-dom/cjs/react-dom.development.js:23447:14)
at Object.<anonymous>.flushWork (node_modules/react-dom/cjs/react-dom-test-utils.development.js:992:10)
at flushWorkAndMicroTasks (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1001:5)
at node_modules/react-dom/cjs/react-dom-test-utils.development.js:1080:11
```
There was a problem hiding this comment.
This is most likely an issue with a jest.mock in the user settings hook unit test.
| @@ -291,6 +291,17 @@ const AppContents: React.FC<{}> = () => { | |||
| /> | |||
| )} | |||
| /> | |||
| <Route | |||
There was a problem hiding this comment.
To make things easier in the future to really see what routes were impacted, I left the new route next to the old route instead of creating a helper.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [lastClusterLoaded]); | ||
| }, [lastClusterLoaded, urlCluster, window.location.pathname]); |
There was a problem hiding this comment.
Putting window.location.pathname seems a little weird, but it works.
- If isMultiClusterEnabled, add the cluster name to '/k8s/ ' based urls - Change import path from store to not be relative
Includes: - renaming "activeCluster" variable in resource-link.tsx - forwarding from a /cluster route to another /cluster route
If
isMultiClusterEnabledis true, this will automatically add the cluster name to the URL string for selected routes.The purpose of this PR is for discussion before merging and finally merging multi-cluster to the feature branch on openshift/console
I did have some concerns and will mark those in the code