Autobind replacement and react-hot-reload update #3022
Conversation
|
Deploy preview for insomnia-storybook ready! Built with commit 41e2a8c |
There was a problem hiding this comment.
Much cleaner PR, and it seems to be working well too. Nice! 🎉
react-fast-refresh isn't stable for Webpack yet, but that would be the next transition when the time comes.
There was a problem hiding this comment.
Thank you for doing this, seems to work great overall!
I noticed that it seems to break some functionality in the sidebar upon hot reloading, but I'll defer to @develohpanda as to whether that's best to fix before merging this or if it can be addressed later.
I'm not sure if we need to also manually ignore some of the lifecycle methods in the files where we use them @develohpanda?
The library automatically ignores these ones: https://github.com/jmrog/class-autobind-decorator/blob/master/src/lib/ReactComponentSpecMethods.js
However, some of them that we use have changed names, and might need to be manually ignored:
UNSAFE_componentWillMount
UNSAFE_componentWillUpdate
UNSAFE_componentWillReceiveProps
Not sure if there are also any new lifecycle methods we use that should be ignored.
Regardless, that could probably be addressed at a future date.
|
In the last commit, I manually added unsafe react lifecycles as to be ignored by the decorator. EDIT: |
There was a problem hiding this comment.
Yes the UNSAFE_* lifecycle methods should be ignored too (and aren't currently as a result of the library being slightly outdated).
Nice work adding them @MrSnix 🎉, although (if it works) I think it might be safer to apply a common autoBindConfig to each instance of the decorator, rather than selectively depending on usage in a component. If specified per component, as it changes over time it is likely someone will forget to update the autoBind config for it.
// common.js or somewhere
const autoBindConfig = {
methodsToIgnore: ['UNSAFE_componentWillReceiveProps', ...],
};
// component file
@autoBindMethodsForReact(autoBindConfig)
class Component { ... }
As requested by @develohpanda, I'm opening this PR as a proposal about issue #2468.
This PR is an improved version from the previous one #2963 which has been closed in favor of this one.
Context
The auto-bind decorator is preventing react-hot-reload from working properly due to binding not only the class functions but even the React lifecycles functions (which is unnecessary and should be avoided) causing the following error:
The current idea
We aim to replace autobind-decorator with class-autobind-decorator that introduces
@autoBindMethodsForReacta convenience decorator that skips auto binding methods from the React Component Spec that do not require binding.Here is a gif that shows react-hot-reloading working again
As always I'm glad to hear feedback.
Thanks
Closes #2468