refactor: enable and fix class-methods-use-this

[tinfoil-knight]

- Summary

See #1891

Enables the class-methods-use-this for ESLint and fixes instances where this wasn't used by turning them to static methods.

- Test plan

npm run format:check:lint runs without error.
npm test passed for all tests.

[tinfoil-knight]

Snapshots for should detect a known framework & should prompt when multiple frameworks are detected are failing. The directory is getting printed along with the npm start command. I'm not able to determine why.

Difference:

    `◈ Netlify Dev ◈␊
    ◈ Starting Netlify Dev with create-react-app␊
    ␊
  - > @ start /private/var/folders/0k/64h0wtf946jbc7dp54bqv73c0000gn/T/netlify-cli-tests-v16.3.0/5163/e6750e7f-4b93-48ae-90e2-022f35c32b40/site-with-cra␊
  + > start␊
    > react-scripts start␊
    ␊
    ◈ "npm start" exited with code *. Shutting down Netlify Dev server`

  › tests/framework-detection.test.js:148:7
  › withSiteBuilder (tests/utils/site-builder.js:177:12)
  › tests/framework-detection.test.js:139:3

[erezrokah]

Hi @tinfoil-knight, let me try and reproduce this locally.
We've seen this happen only on Node.js 10 (and have code to normalize it).

Those tests are passing in our CI on Node.j 16 and 10 though

[tinfoil-knight]

There was a merge conflict in src/utils/command.js. Has been resolved now.

[tinfoil-knight]

I've removed un-necessary multiple merge commits from main.

[tinfoil-knight]

@erezrokah Please merge main and approve running workflow.

[tinfoil-knight]

@erezrokah Please review and merge if everything looks fine.

[erezrokah]

Hi @tinfoil-knight, thanks for opening the PR. This is on my todo list for today 👍

[erezrokah]

Thanks for doing this @tinfoil-knight!
This will help keeping the CLI code nice a tidy 🧹

What do you think of extracting the instance methods that didn't use this to plain functions?

For example in the Command class file, see pollForToken

Then we could export them under utils from the command.js file to be exported in the following manner:

const { utils: { log, logJson, ... } } = require('../utils/command')

That will allow us to follow up in another PR and move non related Command functions to separate files.

[tinfoil-knight]

@erezrokah I wanted to extract those utilities out too. Will do it. How about I put them in a command-utils file instead of removing the default export for BaseCommand. [We can later separate these to different files wherever necessary]

[erezrokah]

@erezrokah I wanted to extract those utilities out too. Will do it. How about I put them in a command-utils file instead of removing the default export for BaseCommand. [We can later separate these to different files wherever necessary]

👍 That makes sense

[tinfoil-knight]

@erezrokah In many places there's code like below where functions (like log) are passed as arguments. For class methods, this seems fine but now that some static methods are being taken out and used separately, passing those functions as parameters seems a bit weird? What do you think?

const logExistingAndExit = ({ log, exit, siteInfo }) => {
  log()
  log(`This site has been initialized`)
  log()
  log(`Site Name:  ${chalk.cyan(siteInfo.name)}`)
  log(`Site Url:   ${chalk.cyan(siteInfo.ssl_url || siteInfo.url)}`)
  log(`Site Repo:  ${chalk.cyan(getRepoUrl({ siteInfo }))}`)
  log(`Site Id:    ${chalk.cyan(siteInfo.id)}`)
  log(`Admin URL:  ${chalk.cyan(siteInfo.admin_url)}`)
  log()
  log(`To disconnect this directory and create a new site (or link to another siteId)`)
  log(`1. Run ${chalk.cyanBright.bold('netlify unlink')}`)
  log(`2. Then run ${chalk.cyanBright.bold('netlify init')} again`)
  exit()
}

For more context, see init.js.

[tinfoil-knight]

@erezrokah Could you please review the question above once when time allows so I can progress forward on the issue?

[erezrokah]

@erezrokah In many places there's code like below where functions (like log) are passed as arguments. For class methods, this seems fine but now that some static methods are being taken out and used separately, passing those functions as parameters seems a bit weird? What do you think?

Good point, could we require those functions instead of passing as arguments?

[tinfoil-knight]

Good point, could we require those functions instead of passing as arguments?

Yep. That's what I was thinking. Just taking the functions out and making it a top-level require/import in the file instead of passing as args.
Just wanted to confirm since things would change in a lot of places. Thank you.

[erezrokah]

Currently, running npx ava tests/serving-functions.test.js fails function-with-included-files with a timeout. This doesn't happen when running with the --serial flag though.

This seems unrelated to your change. Those tests are flaky and we're actively trying to stabilize them

[tinfoil-knight]

This seems unrelated to your change. Those tests are flaky and we're actively trying to stabilize them

Alright then. The PR is ready for a review in that case.

[tinfoil-knight]

I've fixed all the merge conflicts.

[erezrokah]

@tinfoil-knight thanks again for another very important PR.

I'm going to test this manually a bit to make sure various code paths didn't break with all the utilities extractions

[tinfoil-knight]

This PR should also fix #2728.

[erezrokah]

This PR should also fix #2728.

Good point. I think it fixes part of #2728, as we still have warn and error that are used from then oclif command instance.