Skip to content

Fix noMethodSetState url's #2078

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

Closed
JBallin opened this issue Dec 7, 2018 · 3 comments
Closed

Fix noMethodSetState url's #2078

JBallin opened this issue Dec 7, 2018 · 3 comments

Comments

@JBallin
Copy link
Contributor

JBallin commented Dec 7, 2018

@Arcanemagus's docsUrl commit didn't account for the rule naming conventions of the different method names in /lib/util/makeNoMethodSetStateRule.

Currently componentWillUpdate links to /docs/rules/componentWillUpdate (doesn't exist, 404) when it should be linking to /docs/rules/no-will-update-set-state:

function makeNoMethodSetStateRule(methodName, shouldCheckUnsafeCb) {
  return {
    meta: {
      docs: {
        description: `Prevent usage of setState in ${methodName}`,
        category: 'Best Practices',
        recommended: false,
        url: docsUrl(methodName)
      },

I'm proposing to create a function that gets the proper url within /lib/util/makeNoMethodSetStateRule.

I have three suggestions to propose, and wanted feedback prior to opening a PR. Do you recommend placing this function at the top, under the 'Rule Definition' header? And then I would call the function directly within the docs object: url: docsUrl(getDocsTitle(methodName))

function getDocsTitle1(methodName) {
  let title;
  switch (methodName) {
    case 'componentDidMount':
      title = 'did-mount';
      break;
    case 'componentDidUpdate':
      title = 'did-update';
      break;
    case 'componentWillUpdate':
      title = 'will-update';
      break;
    default:
      throw Error(`No docsUrl for '${methodName}'`);
  }
  return `no-${title}-set-state`;
}

function getDocsTitle2(methodName) {
  return `${methodName.match(/[A-Z][a-z]+/g)
    .reduce((res, e) => `${res}-${e.toLowerCase()}`, 'no')}-set-state`;
}

function getDocsTitle3(methodName) {
  return methodName.match(/[A-Z][a-z]+/g)
    .reduce((x, y) => `no-${x.toLowerCase()}-${y.toLowerCase()}-set-state`);
}

function getDocsTitle4(methodName) {
  const map = {
    componentDidMount: 'did-mount',
    componentDidUpdate: 'did-update',
    componentWillUpdate: 'will-update',
  };
  const title = map[methodName];
  if (!title) throw Error(`No docsUrl for '${methodName}'`);
  return `no-${title}-set-state`;
}

console.log(getDocsTitle1('componentWillUpdate')); // no-will-update-set-state
console.log(getDocsTitle2('componentWillUpdate')); // no-will-update-set-state
console.log(getDocsTitle3('componentWillUpdate')); // no-will-update-set-state
console.log(getDocsTitle4('componentWillUpdate')); // no-will-update-set-state
@ljharb
Copy link
Member

ljharb commented Dec 8, 2018

I’d prefer a fourth option, that uses an object map lookup - never a switch, and ideally not string manipulation :-)

@JBallin
Copy link
Contributor Author

JBallin commented Dec 9, 2018

@ljharb great suggestion! Added above (4), let me know if there's anything else I can improve.

@ljharb
Copy link
Member

ljharb commented Dec 10, 2018

A PR with the 4 solution seems like a good place to start, thanks!

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

No branches or pull requests

2 participants