fix: Added prefer-const to eslint and related changes

[aniketkudale]

Fixes #640

[aniketkudale]

Hi @kumar303, sorry for new PR, I messed up in my previous PR so closed that one. Also, the test for commit 6b47da4 was successful on my machine, dont know why it is failing on travis-ci. Once again thanks for taking time for reviewing.

[shubheksha]

Hi @aniketkudale, Travis is complaining because the commit message of your second commit doesn't follow format mentioned in the contributing guide.

[aniketkudale]

Ah, thanks. Noted :)

[kumar303]

Thanks for following up. This looks great, just one more change request.

[kumar303]

This is indented too far. It should look like this:

const watcher: Watchpack = (
  createWatcher({addonId, client, sourceDir, artifactsDir})
);

This provides a visual indication to the reader that the block is connected to the line opening with const watcher....

[aniketkudale]

Thanks a lot @kumar303 I'll update the change.

[kumar303]

In regards to the commit message formatting problem, are you using git commit --no-verify ? Because git should not let you commit with a message formatted this way. Be sure all your local dependencies are up to date by running npm install.

[aniketkudale]

Actually I am using Tortoise SVN tool for commits, so may be this is causing the problem.

[kumar303]

Huh, seems like that should work. I see that TortoiseGit has custom hooks but I don't see why it would ignore our git hooks.

[kumar303]

Let me know if you need help fixing the Flow errors

[aniketkudale]

Yes, please! This looks interesting. Neither of cmds, npm run build and npm run lint, throw any warning on my machine.

[aniketkudale]

@kumar303 Will it be ok to use var, this doesnt give any warnings/error on npm run build and npm run lint

  for (const pref of cliPrefs) {
    var [key, value] = pref.split('='); // using var here, line 137 (src\firefox\preferences.js)

    if (/[^\w{@}.-]/.test(key)) {
      throw new UsageError(`Invalid custom preference name: ${key}`);
    }

    if (value === `${parseInt(value)}`) {
      value = parseInt(value, 10);
    } else if (value === 'true' || value === 'false') {
      value = (value === 'true');
}

[kumar303]

Hi, thanks for the quick response.

Neither of cmds, npm run build andnpm run lint, throw any warning on my machine.

You should be using the npm start command to develop with. When you get a chance, please read through the development process to get familiar with it: https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md

Will it be ok to use var?

All variables that do not get modified must be declared with const. For any variable that needs to be modified, use let because it deals with scoping better than var. But, yes, the problem you are running into is that you changed a variable from let to const but that variable is getting modified so it can't be a constant.

[aniketkudale]

oh, @kumar303 so can we write
const [key value] = pref.split('=');
as

const [key] = pref.split('=');
let [value] = pref.split('=');

[rpl]

@aniketkudale you should be able to rewrite it as:

const prefsAry = pref.split('=');
const [key] = prefsAry;
let [, value] = prefsAry;

The comma (,) in second line means that you want to skip the first element and assign the second one to value.

Nevertheless, I'm not sure that it is very readable written in this way, I would not be against using the regular syntax in this case, eg.:

const prefsAry = pref.split('=');
const key = prefsAry[0];
let value = prefsAry[1];

[aniketkudale]

Thanks @rpl for reply.

const prefsAry = pref.split('=');
const key = prefsAry[0];
let value = prefsAry[1];

This looks readable & simple to understand.

[coveralls]

Coverage remained the same at 100.0% when pulling 7e8b20f on aniketkudale:pref-const-fix into 5e47787 on mozilla:master.

[kumar303]

Thanks! This style check will be very helpful for other contributors. You can ignore the changelog lint failures, we'll be fixing that in #701

[aniketkudale]

Thanks a lot @kumar303 for merging my PR.