fix: move getServerRoute to selectors module to avoid TDZ on plugin init#84
Conversation
|
@cpoile @hanzei @mickmister It looks like I don't have permission to add any reviewers. Would one of you mind taking a look at this when you have a chance? Thanks! |
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
Nice improvement! Just listed a couple of suggestions that would be nice to have alongside this fix
| export const getServerRoute = (state) => { | ||
| const config = getConfig(state); | ||
|
|
||
| let basePath = ''; | ||
| if (config && config.SiteURL) { | ||
| basePath = new URL(config.SiteURL).pathname; | ||
|
|
||
| if (basePath && basePath[basePath.length - 1] === '/') { | ||
| basePath = basePath.substr(0, basePath.length - 1); | ||
| } | ||
| } | ||
|
|
||
| return basePath; | ||
| }; |
There was a problem hiding this comment.
It might be nice to have a test suite for this selector specifically as well
There was a problem hiding this comment.
@avasconcelos114 I added selector tests.
| if (config && config.SiteURL) { | ||
| basePath = new URL(config.SiteURL).pathname; | ||
|
|
||
| if (basePath && basePath[basePath.length - 1] === '/') { | ||
| basePath = basePath.substr(0, basePath.length - 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
It might be nice to wrap this in a try-catch block to prevent any possible runtime errors due to a malformed SiteURL
There was a problem hiding this comment.
@avasconcelos114 I wrapped this in a try/catch block.
…ed site URL values.
avasconcelos114
left a comment
avasconcelos114
left a comment
There was a problem hiding this comment.
LGTM! Thanks for making the changes!
2 participants
Summary
Move
getServerRoutefromindex.jsinto a newselectors.jsmodule and import it, mirroring the structure of the GitHub, GitLab, and Jira plugins. Fixes aReferenceError: Cannot access 'getServerRoute' before initializationthrown fromPlugin.initialize()on Mattermost v11.x, which hangs the webapp on the loading spinner.Also adds a regression test that mocks
window.registerPluginto synchronously invokeinitialize()(mirroring modern Mattermost) and asserts the module loads without throwing. Temporarily reintroducing the original inlineconstdeclaration inindex.jscauses the test to fail with the sameReferenceErrorseen at runtime.Ticket Link
Fixes #83