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.

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

fix: Avoid dirname for built-in configs. #15616

Merged
merged 1 commit into from Feb 25, 2022

Conversation

daidodo
Copy link
Contributor

@daidodo daidodo commented Feb 16, 2022

Load eslint:recommended and eslint:all configs via import instead file paths.

Fixes: #15575

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

  • Documentation update
  • Bug fix (template)
  • New rule (template)
  • Changes an existing rule (template)
  • Add autofix to a rule
  • Add a CLI option
  • Add something to the core
  • Other, please explain: Avoid using __dirname for built-in configs, i.e. eslint:recommended and eslint:all.

What changes did you make? (Give an overview)

I made changes to cli-engine.js and file-enumerator.js to pass in callbacks for getting built-in configs instead of file paths.

Is there anything you'd like reviewers to focus on?

The main changes are in @eslint/eslintrc (PR).
It should be merged and released first before changes here can be applied.

Load eslint:recommended and eslint:all configs via import instead file paths.

Fixes: eslint#15575
@eslint-github-bot eslint-github-bot bot added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Feb 16, 2022
Copy link
Member

@nzakas nzakas left a comment

Can you add some tests for this?

@daidodo
Copy link
Contributor Author

daidodo commented Feb 17, 2022

Could you tell me what tests you are expecting? I thought there were no API changes so existing tests should be enough.
Please also note that the tests failed because eslint/eslintrc#71 hasn't been released.
Thanks!

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Feb 18, 2022
@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 23, 2022

We recently added some tests that would fail if this doesn't work at all: bfaa548

Maybe we could add another test in tests/lib/cli-engine/cli-engine.js to check entire configurations, and also make sure that we won't accidentally revert to passing string paths:

it("should pass functions that load built-in configs to CascadingConfigArrayFactory", () => {

    let constructorArgs;

    const eslintrc = require("@eslint/eslintrc");
    const { CLIEngine } = proxyquire("../../../lib/cli-engine/cli-engine", {
        "@eslint/eslintrc": {
            ...eslintrc,
            Legacy: {
                ...eslintrc.Legacy,
                CascadingConfigArrayFactory: class extends CascadingConfigArrayFactory {
                    constructor(...args) {
                        constructorArgs = args;
                        super(...args);
                    }
                }
            }
        }
    });

    // eslint-disable-next-line no-new -- Testing side effects
    new CLIEngine();

    const [options] = constructorArgs;

    assert.strictEqual(typeof options.eslintRecommendedPath, "undefined");
    assert.strictEqual(typeof options.getEslintRecommendedConfig, "function");
    assert.deepStrictEqual(options.getEslintRecommendedConfig(), require("../../../conf/eslint-recommended"));
    assert.strictEqual(typeof options.eslintAllPath, "undefined");
    assert.strictEqual(typeof options.getEslintAllConfig, "function");
    assert.deepStrictEqual(options.getEslintAllConfig(), require("../../../conf/eslint-all"));

});

@nzakas did you mean something like this?

@nzakas
Copy link
Member

nzakas commented Feb 24, 2022

I’d much prefer a simpler test where we check configs with eslint:recommended and eslint:all produce the desired results.

@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 24, 2022

I’d much prefer a simpler test where we check configs with eslint:recommended and eslint:all produce the desired results

There are such tests for the ESLint class with eslint:recommended and eslint:all configs here.

nzakas
nzakas approved these changes Feb 25, 2022
@nzakas
Copy link
Member

nzakas commented Feb 25, 2022

Ah good call. In that case I think we are good. 👍

I’m assuming the lint error will resolve when eslintrc is upgraded.

mdjermanovic added a commit to mdjermanovic/eslint that referenced this pull request Feb 25, 2022
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 25, 2022
@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 25, 2022

I'll close-reopen to run checks with these changes on top of the latest main.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks for contributing!

@mdjermanovic mdjermanovic merged commit cdc5802 into eslint:main Feb 25, 2022
16 of 24 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 8, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.9.0` -> `8.10.0`](https://renovatebot.com/diffs/npm/eslint/8.9.0/8.10.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.10.0`](https://github.com/eslint/eslint/releases/v8.10.0)

[Compare Source](eslint/eslint@v8.9.0...v8.10.0)

#### Features

-   [`6e2c325`](eslint/eslint@6e2c325) feat: Add `ignoreOnInitialization` option to no-shadow rule ([#&#8203;14963](eslint/eslint#14963)) (Soufiane Boutahlil)
-   [`115cae5`](eslint/eslint@115cae5) feat: `--debug` prints time it takes to parse a file ([#&#8203;15609](eslint/eslint#15609)) (Bartek Iwańczuk)
-   [`345e70d`](eslint/eslint@345e70d) feat: Add `onlyOneSimpleParam` option to no-confusing-arrow rule ([#&#8203;15566](eslint/eslint#15566)) (Gautam Arora)

#### Bug Fixes

-   [`cdc5802`](eslint/eslint@cdc5802) fix: Avoid `__dirname` for built-in configs ([#&#8203;15616](eslint/eslint#15616)) (DoZerg)
-   [`ee7c5d1`](eslint/eslint@ee7c5d1) fix: false positive in `camelcase` with combined properties ([#&#8203;15581](eslint/eslint#15581)) (Nitin Kumar)

#### Documentation

-   [`1005bd5`](eslint/eslint@1005bd5) docs: update CLA information ([#&#8203;15630](eslint/eslint#15630)) (Nitin Kumar)
-   [`5d65c3b`](eslint/eslint@5d65c3b) docs: Fix typo in `no-irregular-whitespace` ([#&#8203;15634](eslint/eslint#15634)) (Ryota Sekiya)
-   [`b93af98`](eslint/eslint@b93af98) docs: add links between rules about whitespace around block curly braces ([#&#8203;15625](eslint/eslint#15625)) (Milos Djermanovic)
-   [`ebc0460`](eslint/eslint@ebc0460) docs: update babel links ([#&#8203;15624](eslint/eslint#15624)) (Milos Djermanovic)

#### Chores

-   [`7cec74e`](eslint/eslint@7cec74e) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).2.0 ([#&#8203;15648](eslint/eslint#15648)) (Milos Djermanovic)
-   [`11c8580`](eslint/eslint@11c8580) chore: read `ESLINT_MOCHA_TIMEOUT` env var in Makefile.js ([#&#8203;15626](eslint/eslint#15626)) (Piggy)
-   [`bfaa548`](eslint/eslint@bfaa548) test: add integration tests with built-in configs ([#&#8203;15612](eslint/eslint#15612)) (Milos Djermanovic)
-   [`39a2fb3`](eslint/eslint@39a2fb3) perf: fix lazy loading of core rules ([#&#8203;15606](eslint/eslint#15606)) (Milos Djermanovic)
-   [`3fc9196`](eslint/eslint@3fc9196) chore: include `tests/conf` in test runs ([#&#8203;15610](eslint/eslint#15610)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1190
Reviewed-by: 6543 <6543@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
srijan-deepsource pushed a commit to deepsourcelabs/eslint that referenced this pull request May 30, 2022
Load eslint:recommended and eslint:all configs via require instead file paths.

Fixes: eslint#15575
srijan-deepsource added a commit to deepsourcelabs/eslint that referenced this pull request May 30, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 25, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: ESLint API cannot find eslint-recommended.js
3 participants