-
Notifications
You must be signed in to change notification settings - Fork 659
OCPBUGS-60969: Console headers are empty when used from a dynamic plugin #15439
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
Changes from 3 commits
1e9aced
cc32506
9a32ca7
04696de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,9 @@ const store = createStore( | |
composeEnhancers(applyMiddleware(thunk)), | ||
); | ||
|
||
// Make store globally available for dynamic plugins | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on my research and learning, I found the different And this leads to the different store to be used. By setting the store to the shared global space, we can make sure that same store get used. This is my understanding, what do you think? @TheRealJon There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Leo6Leo, we've always had a singleton Redux store instance. It's already declared globally in the storeHandler you referenced. @vojtechszocs could you clarify how this should work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The root cause of the problem seems to be that So I think the only change we need for this bugfix is to modify // ...
import storeHandler from '@console/dynamic-plugin-sdk/src/app/storeHandler';
// ...
storeHandler.setStore(store);
export default store; Bugfix PRs should contain minimal changes to address the immediate issue. We can further improve the Redux handling code in follow-up non-bugfix PRs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After applying @vojtechszocs 's fixI have performed the fix @vojtechszocs suggested locally, I can see that the store is successfully initialized, but that doesn't solve the problem with the Further investigationOne key observation I found is that: whether
![]()
![]() Therefore, I think the root cause I found here is still valid.
And here is a quote from the LLM that I think is a reasonable explanation to me:
So I think the approach of making the store become global makes sense. As suggested, we should only have 1 redux store, should we consider making that store become global? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think if we do fix it this way, it should be commented with a FIXME and a CONSOLE project issue to look further into this. We probably shouldn't leak the internal application state into the window scope as a long-term solution. |
||
(window as any).consoleStore = store; | ||
|
||
export const applyReduxExtensions = (reducerExtensions: ResolvedExtension<ReduxReducer>[]) => { | ||
const pluginReducers: ReducersMapObject = {}; | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.