Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable the ESLint no-shadow rule #11745

Merged
merged 3 commits into from
Mar 25, 2020
Merged

Enable the ESLint no-shadow rule #11745

merged 3 commits into from
Mar 25, 2020

Commits on Mar 25, 2020

  1. Enable the ESLint no-shadow rule 

    This rule is *not* currently enabled in mozilla-central, but it appears commented out[1] in the ESLint definition file; see https://searchfox.org/mozilla-central/rev/c80fa7258c935223fe319c5345b58eae85d4c6ae/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#238-239

Unfortunately this rule is, for fairly obvious reasons, impossible to `--fix` automatically (even partially) and each case thus required careful manual analysis.
Hence this ESLint rule is, by some margin, probably the most difficult one that we've enabled thus far. However, using this rule does seem like a good idea in general since allowing variable shadowing could lead to subtle (and difficult to find) bugs or at the very least confusing code.

Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-shadow

---
[1] Most likely, a very large number of lint errors have prevented this rule from being enabled thus far.
    @Snuffleupagus
    Snuffleupagus committed Mar 25, 2020
    Configuration menu
    Copy the full SHA
    1d2f787 View commit details
    Browse the repository at this point in the history

  2. Whitelist closure related cases to address the remaining no-shadow … 

    …linting errors

Given the way that "classes" were previously implemented in PDF.js, using regular functions and closures, there's a fair number of false positives when the `no-shadow` ESLint rule was enabled.

Note that while *some* of these `eslint-disable` statements can be removed if/when the relevant code is converted to proper `class`es, we'll probably never be able to get rid of all of them given our naming/coding conventions (however I don't really see this being a problem).
    @Snuffleupagus
    Snuffleupagus committed Mar 25, 2020
    Configuration menu
    Copy the full SHA
    dcb16af View commit details
    Browse the repository at this point in the history

  3. Remove a spurious console.log from the ChromiumBrowser function i… 

    …n `test/webbrowser.js` file

This looks entirely like something which was left-over from debugging, and that line hasn't been touched since PR 4515, especially considering that the corresponding branch in `FirefoxBrowser` doesn't print anything.
    @Snuffleupagus
    Snuffleupagus committed Mar 25, 2020
    Configuration menu
    Copy the full SHA
    fdfcde2 View commit details
    Browse the repository at this point in the history