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

feat: Avoid dirname for built-in configs. #71

Merged
merged 5 commits 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: eslint/eslint#15575

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

Fixes: eslint/eslint#15575
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should set filePath: "" for clarity in logs. Otherwise, it would be filePath of the config where the extends was.

lib/config-array-factory.js Outdated Show resolved Hide resolved
lib/config-array-factory.js Outdated Show resolved Hide resolved
Load eslint:recommended and eslint:all configs via import instead file paths.

Fixes: eslint/eslint#15575
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

I've verified that this works well both with eslint/eslint current main branch (which is important for backward compatibility), and with changes from eslint/eslint#15616.


if (extendName === "eslint:recommended") {
const name = `${ctx.name} » ${extendName}`;

if (getEslintRecommendedConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add in a check here to make sure getEslintRecommendedConfig is a function, and throw an error if not? I can picture this being difficult to debug if someone passes a nonfunction accidentally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

});
}
if (extendName === "eslint:all") {
const name = `${ctx.name} » ${extendName}`;

if (getEslintAllConfig) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for getEslintAllConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

Fixes: eslint/eslint#15575
Load eslint:recommended and eslint:all configs via import instead file paths.

Fixes: eslint/eslint#15575
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@daidodo
Copy link
Contributor Author

daidodo commented Feb 19, 2022

Thanks for the approvals!

May I have write access to the repository to merge the changes?

@daidodo
Copy link
Contributor Author

daidodo commented Feb 21, 2022

Thanks for the approvals!

May I have write access to the repository to merge the changes?

@nzakas @mdjermanovic

@mdjermanovic
Copy link
Member

@daidodo thanks for addressing all reviews, we'll merge the changes.

@nzakas what do you think about these steps:

  1. Make 2 PRs in eslint/eslint: one with, one without changes from eslint/eslint#15616. Both use @eslint/eslintrc from this branch. If the checks are passing, it confirms that these changes support both the new functions and the old string paths (the latter is important for backwards compatibility). These are "do not merge" PRs, just to run checks in order to see if we can proceed with the next step.
  2. Merge this PR.
  3. On the release day, release @eslint/eslintrc.
  4. Update package.json in eslint/eslint to use the new @eslint/eslintrc, and merge that change.
  5. Close-reopen eslint/eslint#15616 to run checks with the new @eslint/eslintrc. Merge if the checks are passing.
  6. Release eslint.

@nzakas
Copy link
Member

nzakas commented Feb 23, 2022

That sounds like a solid plan to me. 👍

mdjermanovic added a commit to eslint/eslint that referenced this pull request Feb 25, 2022
mdjermanovic added a commit to mdjermanovic/eslint that referenced this pull request Feb 25, 2022
@mdjermanovic
Copy link
Member

All checks on eslint/eslint#15641 and eslint/eslint#15643 are passing (Step 1), so I'm merging this now (Step 2).

Steps 3-6 will be later today.

@mdjermanovic
Copy link
Member

All steps are done. These changes are released in ESLint v8.10.0 & @eslint/eslintrc v1.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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