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

what does componentDidMount do for this component and can it be removed? #162

Open
macirex opened this issue Nov 12, 2018 · 2 comments
Open

Comments

@macirex
Copy link

macirex commented Nov 12, 2018

Here's the context:

My team was recently handled down a project which was made on "react": "^16.3.1" and it's using react-notification-system: "^0.2.16". We are currently cleaning the code and applying the eslint linter to fix code smells and bad practices.

Thus we are having a lot of errors of the type: "Using this.refs is deprecated."

I searched all the issues about this and we know that this is going to be fixed on #144.

This is the original code that we received:

class Notification extends React.Component {
    static propTypes = {
        scopeToShow: PropTypes.string.isRequired, // what scope this notification component must listen
        dispatch: PropTypes.func.isRequired,
        message: PropTypes.string,
        scopes: PropTypes.array,
        level: PropTypes.string,
    };
    static defaultProps = {
        message: null,
        scopes: [],
        level: null,
    };

    state = {
        notificationSystem: null,
    };

    componentDidMount() {
        this.setState({ notificationSystem: this.refs.notificationSystem });
    }

    componentDidUpdate() {
        const { message, scopes, scopeToShow } = this.props;
        if (message && scopes && scopes.indexOf(scopeToShow) !== -1) {
            this.addNotification();
        }
    }

    // eslint-disable-next-line no-unused-vars
    addNotification = (event) => {
        const { notificationSystem } = this.state;
        const { message, level, dispatch } = this.props;
        notificationSystem.clearNotifications();
        notificationSystem.addNotification({
            message,
            level,
            position: "tc",
            autoDismiss: "5",
            dismissible: false,
            // children: <div>{this.props.message}</div>
        });
        dispatch(notificationActions.removeNotification());
    };

    render() {
        // ignoring warning because the only way to remove all the styles from the component is to pass style as false
        // eslint-disable-next-line react/style-prop-object
        return <NotificationSystem ref="notificationSystem" style={false} />;
    }
}
const mapStateToProps = (state) => ({
    message: notificationSelectors.getMessage(state),
    level: notificationSelectors.getLevel(state),
    scopes: notificationSelectors.getScopes(state),
});

export default connect(mapStateToProps)(Notification);

Meanwhile, we are trying to fix the error so that it does not show. One of the team proposals has been:

class Notification extends React.Component {
    static propTypes = {
        scopeToShow: PropTypes.string.isRequired, // what scope this notification component must listen
        dispatch: PropTypes.func.isRequired,
        message: PropTypes.string,
        scopes: PropTypes.array,
        level: PropTypes.string,
    };

    static defaultProps = {
        message: null,
        scopes: [],
        level: null,
    };

    componentDidUpdate() {
        const { message, scopes, scopeToShow } = this.props;
        if (message && scopes && scopes.indexOf(scopeToShow) !== -1) {
            this.addNotification();
        }
    }
    // eslint-disable-next-line no-unused-vars

    addNotification = (event) => {
        const { message, level, dispatch } = this.props;
        this.node.clearNotifications();
        this.node.addNotification({
            message,
            level,
            position: "tc",
            autoDismiss: "5",
            dismissible: false,
            // children: <div>{this.props.message}</div>
        });
        dispatch(notificationActions.removeNotification());
    };

    render() {
        const ref = (node) => {
            this.node = node;
            return node;
        };
        // ignoring warning because the only way to remove all the styles from the component is to pass style as false
        // eslint-disable-next-line react/style-prop-object
        return <NotificationSystem ref={ref} style={false} />;
    }

}

const mapStateToProps = (state) => ({

    message: notificationSelectors.getMessage(state),

    level: notificationSelectors.getLevel(state),

    scopes: notificationSelectors.getScopes(state),

});

export default connect(mapStateToProps)(Notification);

Basically we are erasing the componentDidMount part, getting the reference and passing it down to NotificationSystem during render.

Is this code refactor reasonable? componentDidMount is to be deprecated on react but it's still used on the example of the component so we don't know if it's strictly needed

PS: We can't test most of the code as it depends on some backend data in order to trigger the notifications system.

Any help is appreciated. Thanks.

@macirex
Copy link
Author

macirex commented Nov 12, 2018

I tested this on a sandbox and just answered my own question, componentDidMount cannot be removed, it breaks the app. Evidence here:

https://codesandbox.io/s/ykznyonwl9

@macirex
Copy link
Author

macirex commented Nov 12, 2018

We managed to make it work. Here's the link: https://codesandbox.io/s/ykznyonwl9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant