chore(eslint): adopt flat config with React/TS rules and add required plugins
Description of Changes
-
What was changed
- Migrated/expanded the ESLint flat config to define clear scopes for TS/TSX, JS/JSX, and Node tooling files.
- Added and configured React and React Hooks linting (
eslint-plugin-react,eslint-plugin-react-hooks), and standardized environment globals viaglobals. - Introduced centralized
ignorePatternsand file globs for source, Node/tooling, and JS files. - Enabled a curated set of TypeScript rules (
@typescript-eslint), relaxed others towarn, and disabled redundant/legacy rules (e.g.,react/react-in-jsx-scope, baseno-unused-vars). - Added supporting dev dependencies in
package.json(React plugin, Hooks plugin, simple-import-sort, globals).
-
Why the change was made
- To improve lint coverage for React/TypeScript code, reduce false positives, and get consistent Node/browser environments.
- To make the config easier to maintain by separating concerns (app code vs. tooling files) and adopting sensible defaults with targeted overrides.
Checklist
General
- [ ] I have read the Contribution Guidelines
- [ ] I have read the Stirling-PDF Developer Guide (if applicable)
- [ ] I have read the How to add new languages to Stirling-PDF (if applicable)
- [ ] I have performed a self-review of my own code
- [ ] My changes generate no new warnings
Documentation
- [ ] I have updated relevant docs on Stirling-PDF's doc repo (if functionality has heavily changed)
- [ ] I have read the section Add New Translation Tags (for new translation tags only)
UI Changes (if applicable)
- [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR)
Testing (if applicable)
- [ ] I have tested my changes locally. Refer to the Testing Guide for more details.
@jbrunton96 let's see what makes sense.
// Should be checked
"@typescript-eslint/no-inferrable-types": "off",
'@typescript-eslint/await-thenable': 'off',
'@typescript-eslint/no-unsafe-assignment': 'off',
'@typescript-eslint/no-unsafe-return': 'off',
'@typescript-eslint/no-unsafe-call': 'off',
'@typescript-eslint/no-unsafe-member-access': 'off',
'@typescript-eslint/no-unsafe-arguments': 'off',
'@typescript-eslint/no-unsafe-argument': 'off',
'@typescript-eslint/only-throw-error': 'off',
'@typescript-eslint/no-floating-promises': 'off',
'@typescript-eslint/prefer-promise-reject-errors': 'off',
'@typescript-eslint/prefer-optional-chain': 'off',
'@typescript-eslint/no-base-to-string': 'off',
'@typescript-eslint/no-misused-promises': 'off',
@jbrunton96 let's see what makes sense.
Hey Ludy, sorry I've been on holiday the last couple of weeks so not been looking at anything here - thanks for raising this PR 🙂. I'm getting caught up on what's changed while I've been gone, but should have time to look at this properly soon, but I'll put my initial thoughts here.
- I'd prefer us to split this into multiple PRs to do the work you're suggesting here for ease of review - the refactoring you're wanting to do to the config file seems a reasonable idea, but it's difficult to validate whether it's doing the same thing if the same PR also changes around which rules are/aren't running
- Using a standardised environment via globals is definitely the right thing to do - I have a draft PR #4346 which I intended to come back to soon
- I'm definitely in favour of adding linting of React and React Hooks - the existing linting that I added was just to get some base things running to start with to catch the most obvious things, but I'd love to expand the scope over time. That being said, I want to make sure we have a consensus amongst the developers that we want to run extra rules rather than me just choosing what we run. I think it's likely that we will all agree to wanting to run these, but this is again a situation where I think we're better having 1 PR for each set of rules we're adding so we can discuss the merits of each ruleset independently.
- Relaxing rules to
warnlevel concerns me, but it really depends on the implementation. I've worked in codebases before where warnings are allowed, and it's always just meant they're ignored forever and might as well not be enabled. If setting rules towarninstead oferrorjust changes how the lint issue is displayed and the lint step still fails (so merging is disallowed), I'm in favour. - I'm open to using tsconfig to disallow some things, but not sure if we're better off with that or the linter doing the job. It seems simpler to me to have one system to do everything, but if one tool does a better job than the other, I'm open to using it.
I only chose this to avoid displaying too many errors, so as not to scare off @frooodle :D
I actually just wanted to start a discussion, as code hygiene is important to me.
We can happily split the PR; as I said, it's meant to be thought-provoking.
It's true I can be easily scared 😂😂
Hey Ludy we've just had a bit of a discussion about this PR as we've been trying to get through the big list of PRs 😄
I think you and I both care very deeply about code quality and I love that you're pushing this forwards in this and other PRs. You've done a few things in this PR which in recent PRs I've liked the idea of and merged similar changes to the config file - I'm not exactly sure if there's any remaining structural changes you wanted to make in here which aren't already in V2 at this point - please let me know if there's still structural improvements you'd like to make in the ESLint config and I'm happy to discuss them (it's just a little hard to tell with the diff also including new rules being added at the same time).
The main change that this PR still contains is enabling a bunch of new linting rules, from eslint-plugin-react and eslint-plugin-react-hooks. Both of these are things that I'd like to add to the source base, but not in this PR. For any new linting rule that we merge into the source, I want the source base to be fully conformant to it, and that has a tendency of generating hugely complex diffs that are very difficult to review properly - especially the React Hooks linting is likely to cause lots of excessive re-rendering issues in the app (we already have a bunch now which need fixing, which is why I want to use those linting rules). For this reason, whenever I'm adding new linting rules, I'll be doing it as lots of individual commits which can be separately reviewed and dropped for future PRs to come back to. I'm hoping that most of the rules will be simple enough that one commit can cover an entire rule, but we'll see... I'd encourage you that if you fancy doing any addition of linting rules in the future (feel free, but no pressure!) to use a similar structure to increase the likelihood of quick and painless review, because linting PRs are generally quite prone to conflicts and better being short-lived.
Out of interest, is there anything you consider the highest priority that you'd like to see applied to the repo in order to help improve code hygiene and help out devs? I've got no particular order that I want to tackle new linting rules at the moment beyond finding an issue as I'm going along and adding a rule for it at the time, which I don't think is the best approach, I've just not had time to step back and think about the repo as a whole's linting yet.
👆 Any thoughts on my last comment Ludy? If not, I'm inclined to close this PR.
👆 Any thoughts on my last comment Ludy? If not, I'm inclined to close this PR.
Consider this PR as a guide for you – for everyone – in the V2 branch. I'll keep the branch updated; perhaps there will be insights that can be helpful later. I can also switch it to draft if that's better.
Consider this PR as a guide for you – for everyone – in the V2 branch. I'll keep the branch updated; perhaps there will be insights that can be helpful later. I can also switch it to draft if that's better.
Okay cool, yeah I think we should change this to Draft just to make it easier to filter out when reviewing because this won't be able to merge until all the linting/type checking errors it introduces are fixed. I'm happy enough for it to stay open as a starting point for finding new things to start running in the linter.
✅ Frontend License Check Passed
All frontend licenses have been validated and no compatibility warnings were detected.
The frontend license report has been updated successfully.
🚀 V2 Auto-Deployment Complete!
Your V2 PR with the new frontend/backend split architecture has been deployed!
🔗 Direct Test URL (non-SSL) http://185.252.234.121:4533
🔐 Secure HTTPS URL: https://4533.ssl.stirlingpdf.cloud
This deployment will be automatically cleaned up when the PR is closed.
🔄 Auto-deployed because PR title or branch name contains V2/version2/React keywords.