Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Migrate to ESLint 9 with flat config and fix monorepo linting #7418

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

Conversation

better-salmon
Copy link
Contributor

@better-salmon better-salmon commented Mar 13, 2025

What is it?

  • Bug

Description

This PR fixes a significant issue where ESLint wasn't functioning properly (silently crashing) across most of the monorepo files. The changes include:

  1. Migration from legacy ESLint config to ESLint 9 with flat configuration
  2. Improved lint script execution by properly separating local ESLint configs from the root config (ensuring they no longer overlap — errors in IDE and from the script will be the same)
  3. Removal of unnecessary eslint-disable comments throughout the codebase
  4. Fixed all new errors introduced by the newer config that were previously missed while ESLint wasn't working
  5. Use eslint.config.js by default in starters (Fix: [✨] Update your template to include eslint.config.js by default #6115)

These improvements ensure proper linting across the entire codebase and enable better code quality enforcement going forward.

Checklist

  • My code follows the developer guidelines of this project
  • I performed a self-review of my own code
  • I added a changeset with pnpm change
  • I made corresponding changes to the Qwik docs
  • I added new tests to cover the fix / functionality

@better-salmon better-salmon requested review from a team as code owners March 13, 2025 13:54
Copy link

changeset-bot bot commented Mar 13, 2025

🦋 Changeset detected

Latest commit: 3023245

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
create-qwik Patch
@builder.io/qwik-city Patch
@builder.io/qwik Patch
eslint-plugin-qwik Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

pkg-pr-new bot commented Mar 13, 2025

Open in Stackblitz

npm i https://pkg.pr.new/@builder.io/qwik@7418
npm i https://pkg.pr.new/@builder.io/qwik-city@7418
npm i https://pkg.pr.new/eslint-plugin-qwik@7418
npm i https://pkg.pr.new/create-qwik@7418

commit: 3023245

Copy link
Contributor

github-actions bot commented Mar 13, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
qwik-docs ✅ Ready (View Log) Visit Preview 3023245

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

this is great! but some small nitpicks

@@ -110,7 +110,7 @@ export const dbAddUserToApplication = async (email: string, publicApiKey: string
.insert(userApplicationMap)
.values({ userId: user.userId, applicationId: app?.applicationId })
.execute();
} catch (e) {
} catch (_e) {
Copy link
Member

Choose a reason for hiding this comment

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

better to just remove it, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've noticed that the no-unused-vars rule is disabled in several places, so I've disabled it here as well for consistency. In my opinion, it's better to maintain uniform rules across all packages in the monorepo. However, enabling this rule everywhere right now would introduce many errors, so perhaps it's best to address that separately in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

I meant } catch { without the (e)

@better-salmon
Copy link
Contributor Author

@wmertens, I also noticed that your monorepo setup isn't optimally configured. I could set up turborepo and shared ESLint and TypeScript configurations. However, since this would be a sizable PR, I wanted to confirm with you before starting any work 🙂

@maiieul maiieul moved this from Backlog to Waiting For Review in Qwik Development Apr 2, 2025
@maiieul
Copy link
Contributor

maiieul commented Apr 2, 2025

@better-salmon a decision was made in the past to not use Nx to avoid unnecessary overhead. Maybe it would work well, but maybe it could cause bugs that are hard to pinpoint. Same with Turborepo I believe. Also such tooling might turn off potential contributions from devs that don't know the tool in question.

TLDR: I think that if you rebase we can merge your PR :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: Waiting For Review
Development

Successfully merging this pull request may close these issues.

[✨] Update your template to include eslint.config.js by default
3 participants