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: config-helpers package #152

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

feat: config-helpers package #152

wants to merge 19 commits into from

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Feb 6, 2025

Prerequisites checklist

What is the purpose of this pull request?

Created a new config-helpers package

What changes did you make? (Give an overview)

  • Added packages/config-helpers directory.

  • .github/ISSUE_TEMPLATE/bug-report.yml and .github/ISSUE_TEMPLATE/change.yml: Added @eslint/config-helpers label to the issue templates. [1] [2]

  • .github/workflows/release-please.yml: Added steps to publish the @eslint/config-helpers package to npm and JSR, and to post release announcements.

  • .release-please-manifest.json: Added @eslint/config-helpers to the release manifest.

  • README.md: Updated the list of packages to include @eslint/config-helpers.

Related Issues

Refs eslint/eslint#19116

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

  1. I purposely kept the README very light as I think the majority of the docs should be on the website.
  2. I deviated from the RFC with automatic naming of configs as I found the RFC-described way difficult to debug. Instead, user configs now are UserConfig[index] and extended configs are now ExtendedConfig[index]. Using the bracket notation allows us to more clearly indicate the presence of a multidimensional array.
  3. Also a deviation from the RFC is that I kept extends as an array of arrays at maximum instead of infinite nesting. I'd like to see how far we get with this.
  4. I'm not sure if I should also update the eslint/migrate-config package in this PR or a separate one. I opted for a separate one for now, but let me know if you think differently.

@mdjermanovic
Copy link
Member

CI Test Types fails with rollup: not found when building packages/compat. I'm not sure how the same steps in the same environment pass in the ubuntu-latest + Node.js 22.x job but fail in this one.

@mdjermanovic
Copy link
Member

Ah, actually steps are not the same. Test Types runs npm install in package directories, but that somehow triggers the root prepare script. Maybe we could just do npm install in the root instead of in package directories.

test_types:
name: Test Types
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: "lts/*"
- name: npm install and test types (core)
working-directory: packages/core
run: |
npm install
npm run build
npm run test:types
- name: npm install and test types (plugin-kit)
working-directory: packages/plugin-kit
run: |
npm install
npm run build
npm run test:types

@mdjermanovic
Copy link
Member

3. Also a deviation from the RFC is that I kept extends as an array of arrays at maximum instead of infinite nesting. I'd like to see how far we get with this.

Doesn't the current implementation allow infinite nesting?

for (const { indexPath, value: extendsElement } of flatTraverse(
objectExtends,
)) {

For example, defineConfig({ extends: [[[[[{ rules: { semi: 2 } }]]]]] }) works and results in [{ rules: { semi: 2 } }].

Namespace normalization works only at the first level, though.

for (const { indexPath, value: extendsElement } of flatTraverse(
objectExtends,
)) {
const extension = /** @type {Config} */ (extendsElement);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to check if this contains extends key and throw an error saying that nested extends are not allowed. Otherwise, end user would get a template message saying that flat config system doesn't support extends, which might be confusing since their top-level extends passed to defineConfig() are valid.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

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

Successfully merging this pull request may close these issues.

3 participants