1

I learn React and now I dont understand this Eslint warning.

Can someone explain please the Eslint docs don't make sense I see other code doing what I do and still no warning.

If I replace the switch statement in this method getProviders the Eslint warning go away so it have to do with the switch??

enter image description here

This is the code:

import React from 'react';
import '../../styles/profile-page-anonymous.css';
import { compose } from 'recompose';
import { withFirebase } from '../../firebase';
import { AuthUserContext, withAuthorization } from '../../session';
import ChangeName from './ChangeName';
import * as ROLES from '../../constants/roles';

class ProfilePageBase extends React.Component {
    constructor(props) {
        super(props);
        this.state = {
            displayName: props.authUser.displayName ?? '',
        };
        this.nameChange = this.nameChange.bind(this);
        this.getProviders = this.getProviders.bind(this);
    }

    getProviders(authUser) {
        switch (authUser.providerData[0].providerId) {
            case 'google.com': {
                return authUser.providerData[0].email;
            }
            case 'facebook': {
                return authUser.providerData[0].providerId;
            }
            case 'twitter': {
                return authUser.providerData[0].providerId;
            }
            default:
                return 'non';
        }
    }

    nameChange(displayName) {
        this.setState({
            displayName,
        });
    }

    render() {
        let userDetails;
        const { authUser } = this.props;
        const { displayName } = this.state;
        const providers = this.getProviders(authUser);
        if (authUser) {
            userDetails = (
                <div>
                    <div className="profileAnonymous">
                        <table>
                            <tbody>
                                <tr>
                                    <td>Display name: </td>
                                    <td>{displayName}</td>
                                </tr>
                                <tr>
                                    <td>User ID: </td>
                                    <td>{authUser.uid}</td>
                                </tr>
                                <tr>
                                    <td>Signed in with: </td>
                                    <td>{providers}</td>
                                </tr>
                            </tbody>
                        </table>
                    </div>
                    <div>
                        <h2>Change your display name</h2>
                    </div>
                    <div className="profileBoxChangeName">
                        <ChangeName authUser={authUser} callback={this.nameChange} />
                    </div>
                </div>
            );
        } else {
            userDetails = <p>Unable to get user details. Please try refreshing the page.</p>;
        }

        return <div>{userDetails}</div>;
    }
}

const ProfilePageAuth = props => (
    <AuthUserContext.Consumer>
        {authUser => (
            <div className="profilePageAuthenticated">
                <ProfilePageBase {...props} authUser={authUser} />
            </div>
        )}
    </AuthUserContext.Consumer>
);

const condition = authUser => authUser && authUser.roles.includes(ROLES.USER);

const enhance = compose(withAuthorization(condition), withFirebase);
const ProfilePageAuthenticated = enhance(ProfilePageAuth);

export default ProfilePageAuthenticated;
Kid
  • 1,869
  • 3
  • 19
  • 48
  • That screenshot adds zero value, it's just a bunch of smudges. – tadman Nov 05 '20 at 21:39
  • 1
    Case statements don't need, nor should have `{ ... }` surrounding them. That's not how they work and it gives a false sense of containment that doesn't exist. These will fall-through unless a `return` or `break` is specified and it's important to visually represent that. – tadman Nov 05 '20 at 21:40
  • 1
    You can also avoid the whole `bind(this)` dance by declaring function properties like `getProviders: authUser => { ... }`. – tadman Nov 05 '20 at 21:43
  • @tadman The brackets sometimes have a use, though it's not needed in this case: https://stackoverflow.com/a/35746467 – CertainPerformance Nov 05 '20 at 21:45
  • @CertainPerformance For scoping, sure, but here? Nah. – tadman Nov 05 '20 at 21:46

1 Answers1

3

Putting a method on a prototype is usually a good design decision if the method has something to do with the instance it's attached to. If you never reference this, the instance, inside the method, then it's a bit odd to have such a method on the prototype, since it doesn't directly relate to the instance. That's what the linter is warning you about.

Either ignore the linting rule, or consider making it a static method instead:

static getProviders(authUser) {

and

const providers = ProfilePageBase.getProviders(authUser);

You could also make it a standalone function instead.

Also, better than switch, consider something like:

static getProviders(authUser) {
  if (authUser === 'google.com') return authUser.providerData[0].email;
  if (authUser === 'facebook' || authUser === 'twitter') return authUser.providerData[0].providerId;
  return 'non';
}
CertainPerformance
  • 356,069
  • 52
  • 309
  • 320
  • I remove the `bind(this)` in constructor and can still run the app normal and the lint warning is still there wondering why I dont use `this`. Hmmm – Kid Nov 05 '20 at 22:23
  • What about encapsulation private methods thinking in React. Making method static feels like a hack. (I come from Java and .Net) – Kid Nov 05 '20 at 22:26
  • 1
    Use `this` if you need to access the instance. But here, you don't need to access the instance, so you don't use `this`, so using a class method doesn't make a whole lot of sense. If you want it to be private, put `#` in front so it can only be accessed from inside the class. `static #getProviders(authUser)` – CertainPerformance Nov 05 '20 at 22:50