ES6 rewrite

[ghost]

No description provided.

[jeremyn]

@Hainish Do we target a specific version of JavaScript?

[ghost]

@jeremyn We target browsers, not JS versions.

[ghost]

@jeremyn Here are the targeted versions. My changes only affect the WebExtensions one. Please note that legacy Firefox version will be removed soon.

WebExtensions:
https://github.com/EFForg/https-everywhere/blob/master/chromium/manifest.json#L31

Legacy Firefox:
https://github.com/EFForg/https-everywhere/blob/master/src/install.rdf#L24

[jeremyn]

You yourself framed this PR as an ES6 rewrite. Firefox and Chrome both understand var vs const and traditional vs arrow function syntax, so that can't motivate the changes here.

I'm not saying your changes are bad by the way, I just want to make sure we're consistent across the codebase, including the stuff in utils that run against Node. (I understand that consistency is a big reason why @Hainish wants to migrate the utils stuff away from Python, for example.)

[ghost]

@jeremyn https://wesbos.com/let-vs-const/
Arrow functions vs traditional anonymous function syntax is mostly preference but it is much shorter and is more readable to some people including me.

[jeremyn]

I think ES6 features are neat, I'm not disagreeing with you there. But it's possible that @Hainish and/or the EFF target ES5 in which case we shouldn't update stuff to ES6 even if we think it's better.

[ghost]

@jeremyn The code already had let's in it so it clearly targets ES6 or higher.

[yfdyh000]

See also #10813.

[ghost]

@yfdyh000 We already had for...of loops in the codebase so I guess this is OK.

[ghost]

See also #10864.

[Hainish]

Thanks, @koops76. I agree, since we already have let statements in the codebase we're already tied to ES6.

[Hainish]

Otherwise LGTM

[Hainish]

Fix indentation here

[ghost]

@Hainish Done.

[yfdyh000]

TypeError: invalid assignment to const 'storage'

I also see other errors during testing. Please test and submit fixes.

[ghost]

@yfdyh000 Can you send me all errors?

[yfdyh000]

@koops76 Do you ever build and tested it?
https://www.dropbox.com/s/0g4i050j2ivdwv3/https-everywhere-2017.7.5~pre.crx?dl=0

Errors in Chrome 59:

storage.js:3 Uncaught TypeError: Assignment to constant variable.
    at storage.js:3
util.js:1 Uncaught SyntaxError: Identifier 'VERB' has already been declared
    at util.js:1
rules.js:220 Uncaught TypeError: Cannot use 'in' operator to search for 'www.miit.gov.cn' in undefined
    at RuleSets.addUserRule (rules.js:220)
    at loadStoredUserRules (background.js:93)
    at background.js:98
background.js:11 [Deprecation] Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.
loadExtensionFile @ background.js:11
_generated_background_page.html:1 Error in response to tabs.query: ReferenceError: activeRulesets is not defined
    at Object.chrome.tabs.query.tabs [as callback] (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:112:36)
    at updateState (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:108:15)
    at Object.storage.get.item [as callback] (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:52:3)
_generated_background_page.html:1 Error in response to tabs.query: ReferenceError: activeRulesets is not defined
    at Object.chrome.tabs.query.tabs [as callback] (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:112:36)
    at updateState (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:108:15)
    at updateEnabledDisabledUI (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/popup.js:109:18)
    at HTMLDocument.<anonymous> (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/popup.js:159:3)
2_generated_background_page.html:1 Error in response to tabs.query: ReferenceError: activeRulesets is not defined
    at Object.chrome.tabs.query.tabs [as callback] (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:112:36)
    at updateState (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:108:15)
    at chrome.windows.onFocusChanged.addListener (chrome-extension://pdnbogcodofhjeppkgbjpdmjbjcpbdfd/background.js:68:3)

an piece errors in Firefox (Nightly) 56a1:

TypeError: invalid assignment to const `storage' storage.js:3:3
SyntaxError: redeclaration of const VERB  util.js:1:1
invalid 'in' operand this.active_tab_rules  background.js:197
invalid 'in' operand this.active_tab_rules  background.js:197

[jeremyn]

It is easy to see from https://github.com/koops76/https-everywhere/blob/931d729eb68723de824fb8f400f4f7798f4db8e8/chromium/storage.js , particularly

const storage = chrome.storage.local;
if (chrome.storage.sync) {
  storage = chrome.storage.sync;
}

that there is a problem, declaring storage as const and then possibly assigning a new value two lines later.

Also, am I reading it correctly that 931d729 was merged with failing Travis build https://travis-ci.org/EFForg/https-everywhere/builds/250627648 ?

Maybe we might want to

1. roll back the changes in this PR
2. implement some form of ES6 linting (Use ESLint #10845)
3. make the automated tests do linting
4. update code to ES6 slowly as updates are made to individual files

[ghost]

@jeremyn I don't think we should simply roll the changes back, we should certainly do 2 and 3. I will make two follow-up PRs, one fixing the errors, the other adding ESLint.

[ghost]

@jeremyn https://travis-ci.org/EFForg/https-everywhere/builds/250627648 was not my fault. Look carefully at the errors.

[Hainish]

I take responsibility here. All tests were passing last night when I merged. These were merged in after the 2017.7.5 release was made.

[Hainish]

@koops76 are you working on a partial reversion? If not I can start working on this.

[jeremyn]

@koops76 The argument for rolling back is that it's hard to understand the implications of some of these changes. For example in popup.js, you changed if (ruleset.active != ruleset.default_state) to if (ruleset.active !== ruleset.default_state), changing != to !==. Now !== is generally preferred but did you walk through the code to check for edge cases that depend on the casting done by !=? Probably not. A lot of the changes are like that, where they are probably okay but actually involve some code inspection to say for sure.

Moving ES6 changes into linting and requiring it to be done slowly over time makes it so that the ES6 changes will be done by someone who is, presumably, already looking at some specific piece of code and has an active, current understanding of what it does.

(None of this is to blame you by the way, I just think it's safer to spread changes like this out, especially given the various problems that @yfdyh000 has found so far.)

[ghost]

@jeremyn OK, I will revert the changes that may have unexpected results, like == to === and function () { to () => { in the follow-up PR.

[ghost]

@jeremyn These changes should still be rigorously tested, but let's not just roll all of them back simply because they may introduce yet unknown bugs. We can always fix them, and I hope any code is rigorously tested before being introduced into the release.

[Hainish]

@koops76 I suspect this will cause a lot of work to merge into the webextensions branch, and I'd prefer the ES6 changes be rebased from that. Reverting this PR at the moment will fix the problem at hand and give us time to do a more thorough job on ES6 for webextensions, which has changed the chromium/ codebase significantly.

[ghost]

@Hainish Maybe you'll create a branch for these changes? Or should they stay in my repo?

[ghost]

So, I think the consensus is that this PR should be reverted from master at least until #10845 is done. Is that correct?

[Hainish]

@koops76 was there a reason you didn't revert the minimum_chrome_version in #10895?

[ghost]

@Hainish Yes: #10864

[Hainish]

@koops76 I see, thanks. I'll revert all changes except that one.

[Hainish]

Reversion: 9a770ca

[ghost]

@jeremyn I have implemented linting and added a Travis task: #10897