Skip to content
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

We have a bunch of react warnings #12877

Closed
turt2live opened this issue Mar 25, 2020 · 3 comments
Closed

We have a bunch of react warnings #12877

turt2live opened this issue Mar 25, 2020 · 3 comments
Labels
T-Defect T-Task Tasks for the team like planning

Comments

@turt2live
Copy link
Member

turt2live commented Mar 25, 2020

fix it

@turt2live turt2live self-assigned this Mar 25, 2020
@jryans jryans added the T-Task Tasks for the team like planning label Mar 25, 2020
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Mar 31, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
This fixes a common React warning we see. Most of these components should be using constructors instead, however componentDidMount is just as good (and doesn't require converting most of these).

Conversion to classes will be done in a later stage of React warning fixes.

For element-hq/element-web#12877
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Mar 31, 2020
This fixes a common React warning we see. Most of these components should be using constructors instead, however componentDidMount is just as good (and doesn't require converting most of these).

Conversion to classes will be done in a later stage of React warning fixes.

For element-hq/element-web#12877
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Mar 31, 2020
These TODO comments are expected to be fixed ASAP, but until that happens let's minimize the errors in the console for development.

For element-hq/element-web#12877

These all aren't using componentDidMount because they do something which causes application instability if componentDidMount were used. Much of these calls are expected to move into constructors once they are converted to real classes.
@turt2live
Copy link
Member Author

turt2live commented Apr 1, 2020

Some we can't fix:

  • Beautiful DND context

Some which are harder to fix:

  • InteractiveTooltip
    Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
    
    Check the render method of `InteractiveTooltip`.
        in AccessibleButton (created by InteractiveTooltip)
        in InteractiveTooltip (created by FormatButton)
        in FormatButton (created by MessageComposerFormatBar)
        in div (created by MessageComposerFormatBar)
        in MessageComposerFormatBar (created by BasicMessageEditor)
        in div (created by BasicMessageEditor)
        in BasicMessageEditor (created by SendMessageComposer)  
        in div (created by SendMessageComposer)
        in SendMessageComposer (created by MessageComposer)
        in div (created by MessageComposer)
        in div (created by MessageComposer)
        in div (created by MessageComposer)
        in MessageComposer (created by RoomView)
        in div (created by RoomView)
        in div (created by MainSplit)
        in MainSplit (created by RoomView)
        in ErrorBoundary (created by RoomView)
        in main (created by RoomView)
        in RoomView (created by LoggedInView)
        in div (created by LoggedInView)
        in DragDropContext (created by LoggedInView)
        in div (created by LoggedInView)
        in LoggedInView (created by MatrixChat)
        in ErrorBoundary (created by MatrixChat)
        in MatrixChat
    

turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Apr 1, 2020
For element-hq/element-web#12877

Original error:
```
Warning: Render methods should be a pure function of props and state; triggering nested component updates from render is not allowed. If necessary, trigger nested updates in componentDidUpdate.

Check the render method of PersistedElement.
    in PersistedElement (created by AppTile)
    in div (created by AppTile)
    in div (created by AppTile)
    in AppTile (created by AppsDrawer)
    in div (created by AppsDrawer)
    in div (created by AppsDrawer)
    in AppsDrawer (created by AuxPanel)
    in div (created by AutoHideScrollbar)
    in AutoHideScrollbar (created by AuxPanel)
    in AuxPanel (created by RoomView)
    in div (created by RoomView)
    in div (created by MainSplit)
    in MainSplit (created by RoomView)
    in ErrorBoundary (created by RoomView)
    in main (created by RoomView)
    in RoomView (created by LoggedInView)
    in div (created by LoggedInView)
    in DragDropContext (created by LoggedInView)
    in div (created by LoggedInView)
    in LoggedInView (created by MatrixChat)
    in ErrorBoundary (created by MatrixChat)
    in MatrixChat
```
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Apr 1, 2020
turt2live added a commit to matrix-org/matrix-react-sdk that referenced this issue Apr 1, 2020
@turt2live
Copy link
Member Author

We've silenced all the warnings we can (and that we know of), but there's still a bunch of conversion work to be done here. We need to remove/replace all the UNSAFE_componentWillDoSomething calls with their updated lifecycle, but this is difficult because it can introduce major regressions/crashes in the app.

In the interest of time, we're deprioritizing this for now. Once we get closer to a React major version bump, we can pick this up.

@turt2live turt2live removed their assignment Apr 3, 2020
phil-flex pushed a commit to phil-flex/matrix-react-sdk that referenced this issue Apr 14, 2020
This fixes a common React warning we see. Most of these components should be using constructors instead, however componentDidMount is just as good (and doesn't require converting most of these).

Conversion to classes will be done in a later stage of React warning fixes.

For element-hq/element-web#12877
@turt2live
Copy link
Member Author

The remainder of this is basically #15066

t3chguy pushed a commit that referenced this issue Oct 17, 2024
Co-authored-by: github-merge-queue <118344674+github-merge-queue@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect T-Task Tasks for the team like planning
Projects
None yet
Development

No branches or pull requests

2 participants