Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Good practice for react-this-binding-issue #676

Closed
astorije opened this issue Dec 14, 2018 · 3 comments
Closed

Good practice for react-this-binding-issue #676

astorije opened this issue Dec 14, 2018 · 3 comments
Labels
Domain: Documentation Rules or repository tasks related to how to document code.
Milestone

Comments

@astorije
Copy link
Contributor

astorije commented Dec 14, 2018

(Sorry this is not a bug report or feature request, but I figured it would be nice to have this listed somewhere on the repo)

Up until v6.0.0, we were gently riding on a bug of the react-this-binding-issue rule that was not seeing some violation. Imagine the following, minimal (as much as I could) example:

import React, { SFC } from 'react';
import { Checkbox } from 'my-theoretical-component-library';

interface CheckboxListProps {
  items: ReadonlyArray<{ name: string; selected: boolean }>;
  onChange(name: string, checked: boolean): void;
}

export const CheckboxList: SFC<CheckboxListProps> = ({ items, onChange }) => (
  <div>
    {items.map(({ name, selected }) => (
      <Checkbox
        checked={selected}
        key={name}
        label={name}
        onChange={checked => {
          onChange(name, checked);
        }}
      />
    ))}
  </div>
);

Prior to v6, TSLint was okay with that, but after the upgrade:

ERROR: 15:19   react-this-binding-issue  A new instance of an anonymous method is passed as a JSX attribute: checked => {
          onCha...

So, yeah, I am creating a new function each time.
What solution do you recommend for this type of violations (that we have across our codebase)?

As far as I can tell, I could

  • Curry the onChange function to go onChange={onChange(name)} to trump the linter, but the underlying issue of creating a function each time would still be there, and that would make for a sillier API for whoever consumes this.
  • Convert to a class, do this.onChange(checked) { return name => this.props.onChange(name, checked); } and use onChange={this.onChange}, which doesn't force the API to change, but I believe still creates plenty anonymous functions, and prevents us to use nice and small SFCs.
  • Use tslint:disable, but that's just sad.
  • Listen to my intoxicated coworker @amacleay who only half-jokingly suggests using memoizee+curry and use onChange={memoize(curry)(onChange, name)}.

Am I missing something nicer and easier to use that would make the linter happy?

@JoshuaKGoldberg
Copy link

Another quick workaround:

{items.map(({ name, selected }) => {
    const onChange = checked => {
        onChange(name, checked);
    };

    return (
      <Checkbox
        checked={selected}
        label={name}
        onChange={onChange}
      />
    );
})}

@astorije
Copy link
Contributor Author

Right so the following works, but it's just to trump the linter really:

import React, { SFC } from 'react';
import { Checkbox } from 'my-theoretical-component-library';

interface CheckboxListProps {
  items: ReadonlyArray<{ name: string; selected: boolean }>;
  onChange(name: string, checked: boolean): void;
}

export const CheckboxList: SFC<CheckboxListProps> = ({ items, onChange }) => (
  <div>
    {items.map(({ name, selected }) => {
      const onChangeWithName = (checked: boolean) => {
        onChange(name, checked);
      };

      return (
        <Checkbox
          checked={selected}
          key={name}
          label={name}
          onChange={onChangeWithName}
        />
      );
    })}
  </div>
);

@JoshuaKGoldberg JoshuaKGoldberg added Domain: Documentation Rules or repository tasks related to how to document code. and removed Type: Question labels Apr 19, 2019
@JoshuaKGoldberg
Copy link

We've been talking about removing this from the recommended ruleset, since the rule is no longer really good practice (and debatably never was!). #309

For folks who discover this: @astorije is right, we can either do silly things to trump the linter or just... disable this relatively less useful rule!

@IllusionMH IllusionMH added this to the None milestone May 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Documentation Rules or repository tasks related to how to document code.
Projects
None yet
Development

No branches or pull requests

3 participants