Closed lieut-data closed 4 years ago
This solution seems a bit weird to me. I'm wondering if the issue is that the call to registerReducer
is after the the call to registerRootComponent
. https://github.com/mattermost/mattermost-plugin-agenda/blob/b767075889d8ae535fc685b17858695dc213e232/webapp/src/index.js#L16-L22
Is there a way we can verify that this is the issue?
Brilliant, @mickmister! I had assumed it was something deeper in the plugin stack and didn't bother to try digging deeper. Moving the reducer up before registering the component does indeed fix the issue. Thank you!
@lieut-data or @marianunez is there a way to replicate the race condition with the missing element? I'm not sure if I can do anything short of regression testing here.
@DHaussermann, I've added some test steps that had worked for me to the ticket: https://mattermost.atlassian.net/browse/MM-23553.
@lieut-data thanks for steps!
Using the steps - the white screen issue reported does not occur for me. However, I do see several JS errors occurs once the plugin is enabled. It looks like the first one is > Uncaught Error: You attempted to set the key `display_name` with the value `""` on an object that is meant to be immutable and has been frozen.
Please advise if this issue should also be included in this PR. The impact is less severe than the initail problem as the JS error banner can be dismissed and I see no functional impact after this.
@DHaussermann That's the same error the Jira plugin was experiencing, and the fix was the same as the fix in this PR. I'm not sure why it is showing the banner versus crashing. If the banner isn't showing up with the PR's fix, then I would say it is good.
Thanks @mickmister for pointing out the related jira issue.
But, when I disable and re-enable the Agenda plugin built from this fix-default-state
I do see the JS error. This would mean a fix is needed in this repo correct?
@DHaussermann The fix is virtually the same fix here though. Specifically, making sure registerReducer
runs before any other call related to the plugin's redux slice. I'm wondering if there's something different about the two environments. Are you refreshing the page after installing the fixed version of the plugin?
https://github.com/mattermost/mattermost-plugin-jira/pull/416/files
@mickmister I'm not sure I'm explaining clearly. Multiple users in other browser session are seeing the JS error. (Not all of them. Maybe related to what channel they're viewing?) Here are the steps:
@DHaussermann Having an open browser session from the old code will most likely result in an error, because the state has already been affected. According the test steps on the Jira ticket, you should refresh after uninstalling the previous version of the plugin.
Since both versions have the same plugin ID (which will always be the case), uninstalling/reinstalling the plugin on different versions is no different than disabling/enabling the plugin of the same version as far as the active browser session is concerned. So you have to have a fresh browser state with no remnants of the old version of the plugin present.
@mickmister Sorry, for the delay on this. Yes the steps work as expected if you refresh but this is a broader problem. When the plugin is enabled active users will see a JS error. Is this solvable?
I've also noticed this occurs on To Do plugin as well. Maybe we could connect on this to ensure we have the same understanding of the issue and to make sure I understand how the Jira PR is related.
@DHaussermann & @mickmister, I've sent a calendar invite your way in the hopes of discussing this further. I haven't stayed up to sync on the conversation and would like to understand the remaining issues better.
Thanks @lieut-data. I posted in the public channel here as well https://community.mattermost.com/core/pl/s45rkizxobdwdy5szikfbjm51o just in case there is a need for public discussion about this.
Summary
The selectors assume a fully populated initial state, and currently result in resolving a property on an undefined object. By falling back to the default state from the reducer (before Redux itself kicks in), we avoid this race condition and allow normal usage of the selectors.
Ticket Link
https://mattermost.atlassian.net/browse/MM-23553