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

Refactor `Linter` to reduce API surface #9161

Closed
not-an-aardvark opened this Issue Aug 26, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@not-an-aardvark
Copy link
Member

not-an-aardvark commented Aug 26, 2017

I think it would be a good idea to refactor Linter in the near future. The goal is to reduce the amount of undocumented API surface area, and simplify the interactions between core modules to make them easier to maintain. Node.js API users frequently handle Linter instances, and having too much API surface area makes it more likely that users will rely on undocumented behavior, and then have their integrations break when we change the implementation details in the future.

Specific action items:

  • Don't make Linter a subclass of EventEmitter.

    Linter happens to use the EventEmitter API internally, but there's no reason it has to expose the entire EventEmitter API by being a subclass. We don't want users to use Linter instances in the same way that they would use generic EventEmitters (for example, we don't want API users to do something like linter.emit("foo", node) or linter.on("foo", listener) directly). Instead, one option would be to store an instance of EventEmitter as a private property. Another (possibly better) option would be to use a new EventEmitter for each verify call.

  • Don't give RuleContext objects access to the underlying Linter instance.

    Currently, rules can call methods on the Linter instance by accessing context.eslint. This allows them to interfere with other rules, or report problems as other rules (e.g. with context.eslint.report("some-other-rule", ...).

    I think it would be better to have the RuleContext constructor accept a report function as an argument. That way, it wouldn't need a reference to Linter at all -- it would just be able to emit reports.

  • Make Linter#verify be side-effect-free.

    Instances of Linter are mutable, and it's probably too late to change that. However, I think it would be good to prevent Linter#verify from having side-effects. To do this, we would get rid of properties like linter.messages, linter.currentConfig, linter.sourceCode, etc. and pass the values around internally instead. We would also remove methods like getAncestors from Linter (and provide an equivalent replacement elsewhere to avoid changing the rule API), since getAncestors necessarily involves some hidden state about the current traversal point, and it doesn't make sense for users of ESLint's Node API to call getAncestors directly.

not-an-aardvark added a commit that referenced this issue Aug 27, 2017

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using internal Linter APIs in RuleTester (refs #9161)
This updates `RuleTester` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in astUtils tests (refs #9161)
This updates the tests for `ast-utils` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in SourceCode tests (refs #9161)
This updates the tests for `SourceCode` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in Linter tests (refs #9161)
This updates the tests for `Linter` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using internal Linter APIs in RuleTester (refs #9161)
This updates `RuleTester` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in astUtils tests (refs #9161)
This updates the tests for `ast-utils` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in SourceCode tests (refs #9161)
This updates the tests for `SourceCode` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in Linter tests (refs #9161)
This updates the tests for `Linter` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using internal Linter APIs in RuleTester (refs #9161) (#…
…9172)

This updates `RuleTester` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in SourceCode tests (refs #9161)…
… (#9174)

This updates the tests for `SourceCode` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

Chore: avoid using private Linter APIs in Linter tests (refs #9161) (#…
…9175)

This updates the tests for `Linter` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

not-an-aardvark added a commit that referenced this issue Aug 28, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

Chore: avoid using private Linter APIs in astUtils tests (refs #9161) (
…#9173)

This updates the tests for `ast-utils` to use the public `Linter#defineRule` API rather than the private `Linter#on` API.

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

not-an-aardvark added a commit that referenced this issue Aug 30, 2017

Chore: remove currentScopes property from Linter instances (refs #9161)
The `currentScopes` property is undocumented, and is redundant with the `scopeManager` property. This commit removes it.

not-an-aardvark added a commit that referenced this issue Sep 8, 2017

not-an-aardvark added a commit that referenced this issue Sep 8, 2017

not-an-aardvark added a commit that referenced this issue Sep 8, 2017

Chore: remove internal Linter#getDeclaredVariables method (refs #9161)
This updates `Linter` to remove the `Linter#getDeclaredVariables` method. The `context.getDeclaredVariables` method is still available to rules -- this just removes the version of the method on `Linter`.

not-an-aardvark added a commit that referenced this issue Sep 8, 2017

not-an-aardvark added a commit that referenced this issue Sep 8, 2017

Chore: remove internal Linter#getDeclaredVariables method (refs #9161)
This updates `Linter` to remove the `Linter#getDeclaredVariables` method. The `context.getDeclaredVariables` method is still available to rules -- this just removes the version of the method on `Linter`.

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

Chore: remove internal Linter#getDeclaredVariables method (refs #9161)
This updates `Linter` to remove the `Linter#getDeclaredVariables` method. The `context.getDeclaredVariables` method is still available to rules -- this just removes the version of the method on `Linter`.

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

Chore: remove internal Linter#getDeclaredVariables method (refs #9161) (
#9264)

This updates `Linter` to remove the `Linter#getDeclaredVariables` method. The `context.getDeclaredVariables` method is still available to rules -- this just removes the version of the method on `Linter`.

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

Chore: remove extraneous linter properties (refs #9161)
This removes the undocumented `traverser`, `scopeManager`, and `currentConfig` properties from `Linter` instances.

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

Chore: remove extraneous linter properties (refs #9161) (#9267)
This removes the undocumented `traverser`, `scopeManager`, and `currentConfig` properties from `Linter` instances.

not-an-aardvark added a commit that referenced this issue Sep 9, 2017

not-an-aardvark added a commit that referenced this issue Sep 10, 2017

not-an-aardvark added a commit that referenced this issue Sep 11, 2017

not-an-aardvark added a commit that referenced this issue Sep 27, 2017

kaicataldo added a commit that referenced this issue Sep 29, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

not-an-aardvark commented Oct 24, 2017

Status of this issue:

  • Most undocumented properties have been removed from Linter.
  • There are still a few undocumented properties (rules and environments) that I'd like to remove. However, these properties are used in other parts of the codebase, so removing them will require some more refactoring.
  • Linter#verify is mostly side-effect-free, except for the fact that it changes the value of Linter#getSourceCode. We can't remove Linter#getSourceCode right now because it's documented here. We could remove it in a major release, but we would probably need to create a usable replacement for it first. (For example, the ESLint demo currently uses Linter#getSourceCode because it's the only good way to get a SourceCode instance for the text being linted.)
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Oct 22, 2018

@not-an-aardvark are you still working on this?

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

not-an-aardvark commented Oct 22, 2018

Not really, I'll close it.

not-an-aardvark added a commit that referenced this issue Jan 30, 2019

Chore: remove undocumented `Linter.rules` property (refs #9161)
This removes the undocumented `rules` property from `Linter` instances, as part of the effort to remove undocumented API surface from `Linter` (also see #9161). Config processing now exclusively uses `Linter`'s public API when defining rules. As a side-effect, the rule map utility in `lib/rules.js` no longer needs to access the filesystem, so we can remove the odd code generation logic from the browserify build.

not-an-aardvark added a commit that referenced this issue Jan 30, 2019

Chore: remove undocumented `Linter#rules` property (refs #9161) (#11335)
This removes the undocumented `rules` property from `Linter` instances, as part of the effort to remove undocumented API surface from `Linter` (also see #9161). Config processing now exclusively uses `Linter`'s public API when defining rules. As a side-effect, the rule map utility in `lib/rules.js` no longer needs to access the filesystem, so we can remove the odd code generation logic from the browserify build.

PlayMa256 added a commit to PlayMa256/GrCartuchos that referenced this issue Feb 1, 2019

Update eslint in group default to the latest version 🚀 (#125)
## The devDependency [eslint](https://github.com/eslint/eslint) was updated from `5.12.1` to `5.13.0`.
This version is **not covered** by your **current version range**.

If you don’t accept this pull request, your project will work just like it did before. However, you might be missing out on a bunch of new features, fixes and/or performance improvements from the dependency update.

---

<details>
<summary>Release Notes for v5.13.0</summary>

<ul>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/91c8884971f5e57f5f7490d8daf92c4a9a489836"><code>91c8884</code></a> Chore: use local function to append "s" instead of a package (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="400953921" data-permission-text="Issue title is private" data-url="eslint/eslint#11293" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11293/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11293">#11293</a>) (Timo Tijhof)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/b5143bfc09e53d8da8f63421ade093b7593f4f51"><code>b5143bf</code></a> Update: for-direction detection false positives/negatives (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="397135133" data-permission-text="Issue title is private" data-url="eslint/eslint#11254" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11254/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11254">#11254</a>) (Ruben Bridgewater)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/9005e632d13476880c55f7e3c8a6e450762a5171"><code>9005e63</code></a> Chore: increase camelcase test coverage (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="401174018" data-permission-text="Issue title is private" data-url="eslint/eslint#11299" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11299/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11299">#11299</a>) (Redmond Tran)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/5b14ad1003c7df9a37621dea55c6d6d0484adc05"><code>5b14ad1</code></a> Fix: false positive in no-constant-condition (fixes <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="401916289" data-permission-text="Issue title is private" data-url="eslint/eslint#11306" data-hovercard-type="issue" data-hovercard-url="/eslint/eslint/issues/11306/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/issues/11306">#11306</a>) (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="402047796" data-permission-text="Issue title is private" data-url="eslint/eslint#11308" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11308/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11308">#11308</a>) (Pig Fang)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/6567c4f6665df85c3347388b29d8193cc8208d63"><code>6567c4f</code></a> Fix: only remove arrow before body in object-shorthand (fixes <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="401807378" data-permission-text="Issue title is private" data-url="eslint/eslint#11305" data-hovercard-type="issue" data-hovercard-url="/eslint/eslint/issues/11305/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/issues/11305">#11305</a>) (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="402033639" data-permission-text="Issue title is private" data-url="eslint/eslint#11307" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11307/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11307">#11307</a>) (Pig Fang)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/fa2f370affa4814dbdda278f9859d0172d4b7aa2"><code>fa2f370</code></a> Docs: update rule configuration values in examples (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="404018485" data-permission-text="Issue title is private" data-url="eslint/eslint#11323" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11323/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11323">#11323</a>) (Kai Cataldo)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/0a3c3ff1d91e8f39943efc4a7d2bf6927d68d37e"><code>0a3c3ff</code></a> New: Allow globals to be disabled/configured with strings (fixes <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="294234548" data-permission-text="Issue title is private" data-url="eslint/eslint#9940" data-hovercard-type="issue" data-hovercard-url="/eslint/eslint/issues/9940/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/issues/9940">#9940</a>) (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="405027784" data-permission-text="Issue title is private" data-url="eslint/eslint#11338" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11338/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11338">#11338</a>) (Teddy Katz)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/dccee63cf41234180c71bf0fe01b165c9078fc69"><code>dccee63</code></a> Chore: avoid hard-coding the list of core rules in eslint:recommended (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="404605578" data-permission-text="Issue title is private" data-url="eslint/eslint#11336" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11336/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11336">#11336</a>) (Teddy Katz)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/c1fd6f54d92efe615bcae529006221e122dbe9e6"><code>c1fd6f5</code></a> Chore: remove undocumented <code>Linter#rules</code> property (refs <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="253051999" data-permission-text="Issue title is private" data-url="eslint/eslint#9161" data-hovercard-type="issue" data-hovercard-url="/eslint/eslint/issues/9161/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/issues/9161">#9161</a>) (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="404599049" data-permission-text="Issue title is private" data-url="eslint/eslint#11335" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11335/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11335">#11335</a>) (Teddy Katz)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/36e335681d61cbe3c83b653b7cc5f95730f1d86e"><code>36e3356</code></a> Chore: remove dead code for loading rules (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="404588598" data-permission-text="Issue title is private" data-url="eslint/eslint#11334" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11334/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11334">#11334</a>) (Teddy Katz)</li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/c464e2744ec76e7e9c6c5af0f6162c92187f1ece"><code>c464e27</code></a> Docs: Rename <code>result</code> -&gt; <code>foo</code> (<a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="394168276" data-permission-text="Issue title is private" data-url="eslint/eslint#11210" data-hovercard-type="pull_request" data-hovercard-url="/eslint/eslint/pull/11210/hovercard" href="https://urls.greenkeeper.io/eslint/eslint/pull/11210">#11210</a>) (Alexis Tyler)</li>
</ul>
</details>

<details>
<summary>Commits</summary>
<p>The new version differs by 13 commits.</p>
<ul>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/4b267a5c8a42477bb2384f33b20083ff17ad578c"><code>4b267a5</code></a> <code>5.13.0</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/720dce11ed89ce17ce3600044fd8e45145d353eb"><code>720dce1</code></a> <code>Build: changelog update for 5.13.0</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/91c8884971f5e57f5f7490d8daf92c4a9a489836"><code>91c8884</code></a> <code>Chore: use local function to append "s" instead of a package (#11293)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/b5143bfc09e53d8da8f63421ade093b7593f4f51"><code>b5143bf</code></a> <code>Update: for-direction detection false positives/negatives (#11254)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/9005e632d13476880c55f7e3c8a6e450762a5171"><code>9005e63</code></a> <code>Chore: increase camelcase test coverage (#11299)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/5b14ad1003c7df9a37621dea55c6d6d0484adc05"><code>5b14ad1</code></a> <code>Fix: false positive in no-constant-condition (fixes #11306) (#11308)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/6567c4f6665df85c3347388b29d8193cc8208d63"><code>6567c4f</code></a> <code>Fix: only remove arrow before body in object-shorthand (fixes #11305) (#11307)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/fa2f370affa4814dbdda278f9859d0172d4b7aa2"><code>fa2f370</code></a> <code>Docs: update rule configuration values in examples (#11323)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/0a3c3ff1d91e8f39943efc4a7d2bf6927d68d37e"><code>0a3c3ff</code></a> <code>New: Allow globals to be disabled/configured with strings (fixes #9940) (#11338)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/dccee63cf41234180c71bf0fe01b165c9078fc69"><code>dccee63</code></a> <code>Chore: avoid hard-coding the list of core rules in eslint:recommended (#11336)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/c1fd6f54d92efe615bcae529006221e122dbe9e6"><code>c1fd6f5</code></a> <code>Chore: remove undocumented <code>Linter#rules</code> property (refs #9161) (#11335)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/36e335681d61cbe3c83b653b7cc5f95730f1d86e"><code>36e3356</code></a> <code>Chore: remove dead code for loading rules (#11334)</code></li>
<li><a href="https://urls.greenkeeper.io/eslint/eslint/commit/c464e2744ec76e7e9c6c5af0f6162c92187f1ece"><code>c464e27</code></a> <code>Docs: Rename <code>result</code> -&gt; <code>foo</code> (#11210)</code></li>
</ul>
<p>See the <a href="https://urls.greenkeeper.io/eslint/eslint/compare/faf3c4eda0d27323630d0bc103a99dd0ecffe842...4b267a5c8a42477bb2384f33b20083ff17ad578c">full diff</a></p>
</details>

<details>
  <summary>FAQ and help</summary>

  There is a collection of [frequently asked questions](https://greenkeeper.io/faq.html). If those don’t help, you can always [ask the humans behind Greenkeeper](https://github.com/greenkeeperio/greenkeeper/issues/new).
</details>

---


Your [Greenkeeper](https://greenkeeper.io) bot 🌴
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment