fix: ESLint config #1025

gminetoma · 2025-02-09T08:12:51Z

✅ Resolves #982

PR title follows Conventional Commits specifications
PR assignee has been selected
PR label has been selected
@NabbeunNabi has been selected for preliminary review

🔧 What Changed

We are now using the correct recommended TSESLint rules (tseslint.configs.eslintRecommended.rules).

Previously, we were using the wrong plugin property: tseslint.configs.recommended. This includes not only the rules but the entire recommended plugin configuration.

Added Vitest ESLint Plugin and Fixed the Following Errors:

Styling
- Ran yarn lint --fix and manually fixed max-statements-per-line.
no-console
- Updated this rule to only affect console.log.
- console.warn, console.error and console.info can still be used.
- In the future, this rule should block all console commands once a logger is added.
no-underscore-dangle
- Added ESLint ignore and renamed variables.
no-shadow
- Renamed conflicting variables.
consistent-return
- Removed the rule due inconsistencies across the codebase.
no-alert
- Added eslint-disable-next-line no-alert

Notes:

I didn't remove the require-atomic-updates warning. This needs further investigation—it may just be a false alarm.

netlify · 2025-02-09T08:13:41Z

✅ Deploy Preview for findadoc ready!

Name	Link
🔨 Latest commit	`fbef2f0`
🔍 Latest deploy log	https://app.netlify.com/sites/findadoc/deploys/67aa288e6d5f50000839d949
😎 Deploy Preview	https://deploy-preview-1025--findadoc.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

socket-security · 2025-02-09T08:14:17Z

No dependency changes detected. Learn more about Socket for GitHub ↗︎

👍 No dependency changes detected in pull request

NabbeunNabi

@gminetoma Great work.

Left a few comments. A couple questions and then I will passed it off to Phil

NabbeunNabi · 2025-02-09T08:18:54Z

eslint.config.js

@@ -62,7 +62,8 @@ export default withNuxt(
            ],
            'vars-on-top': 'off',
            yoda: ['error', 'never', { exceptRange: true }],
-            'no-console': 'error', // we should use the logger instead
+            // Change to a logger in the future


Do you want to write a proposal and research what logger can be implemented. Give a few choices as to what you found and the benefits to each. You can use the template in the gitbook. @ermish does that sound good?

Or @ermish do you already have one you like?

We already have a specific one to use. It's with Grafana / Prometheus

NabbeunNabi · 2025-02-09T08:21:42Z

stores/facilitiesStore.ts

+            if (!data) {
+                return
+            }


The new linting rules want this styling? I like it as long as it's consistent. Also probably more readable for people learning a codebase. 😎

I didn't change any rules besides the no-console

@gminetoma you actually added a bunch of rules by adding the recommended ones. Many times they don't fit our codebases style and what we agreed upon. I haven't read through the rest but our two choices are either inherit from recommended then override the specific ones or stop using recommended and just list out the rules. Usually there's not that many that are needed

Personally for this one, I like simple returns and would like to change this rule back.

@ermish

My intention was to apply the correct recommended rules since ...tseslint.configs.recommended was only being applied in the rules, which was denying all the rules in the block.

I just fixed it, but if this is not what was intended, we can remove it.

I didn't touch this rule. It was being applied 8 months ago when ESLint was working.

I didn't modify any styling rules. This rule comes from here:

We can override it if you approve.

Let's add an override and use the simpler way 👍

Sorry if I wasn't clear, I meant even though you didn't add any specific rules, this PR still does enable rules that were not being linted before it.

Should I make this a rule, or can we leave it up to the developer to decide? What format do you prefer?

if (condition) return

if (condition) { return }

NabbeunNabi · 2025-02-09T08:23:53Z

stores/locationsStore.ts

-        alert('Error getting data! Please contact our support team by clicking the bottom right link on the page!')
+        console.error('Error getting data! Please contact our support team by clicking the bottom right link on the page!')


Nice! This alert would cause a 500 on yarn dev:localserver if the backend wasn't running

Wait. This was intentionally an alert because it's saying our whole site has crashed and we want to tell the user. We could replace this with a modal as an alternative, but console won't work here.

We also have a console error in the line before to give more dev details which this is now duplicating

stores/localeStore.ts

stores/moderationSubmissionsStore.ts

utils/handleScroll.ts

eslint.config.js

NabbeunNabi

@ermish On to you. I tagged you in a couple of my comments

NabbeunNabi · 2025-02-09T10:53:01Z

@ermish On to you. I tagged you in a couple of my comments

Actually I'm going to use my judgment to say I want to have return there but not have to explicitly say I defined. So not yet to you

NabbeunNabi

Actually, I changed my mind. Sorry to do that. Could you see if it is possible to update the config to not require explicitly stating undefined. I am fine with there being a return. But adding undefined seems extra, like a semicolon and we don't use those

NabbeunNabi

Ok @ermish now it is all yours

ermish

Human linting of the PR complete!

ermish · 2025-02-10T01:51:27Z

eslint.config.js

+        },
+        settings: {
+            vitest: {
+                typecheck: true


I don't think we want this feature turned on. It's going to run tsc which would duplicate the typescript Linting and slow things down.

This feature is for Vitest if type checking is enabled in its config.
Since it's not, I'll remove it.

https://github.com/vitest-dev/eslint-plugin-vitest?tab=readme-ov-file#enabling-with-type-testing
https://vitest.dev/guide/testing-types

ermish · 2025-02-10T01:52:19Z

i18n/checkLocaleKeys.js

 import fs from 'fs'
 import path from 'path'
 import { fileURLToPath } from 'url'

 // Convert import.meta.url to __dirname equivalent
-const __filename = fileURLToPath(import.meta.url)
-const __dirname = path.dirname(__filename)
+const fileName = fileURLToPath(import.meta.url)


These are reserved names and should be kept.

Should I keep it or remove it? These names are not available in ES modules.

We have rules that block the usage of "_" for variable naming.
Even if we were running a .cjs script, we shouldn't modify them.

They're well known names in the JavaScript/node space. The linting is from new rules added in this PR that are throwing the error so we should just override the rule.
I think the rule can be ok if it allows exceptions for any other reserved words/special cases

I added this file to the ignore list since it doesn't follow a bunch of our rules.
I also reverted all its changes.

i18n/checkLocaleKeys.js

ermish · 2025-02-10T02:05:52Z

stores/searchResultsStore.ts

@@ -100,7 +100,7 @@ async function queryProfessionals(searchSpecialty?: Specialty, searchLanguage?:
        return professionalsSearchResult
    } catch (error) {
        console.error(`Error getting professionals: ${JSON.stringify(error)}`)
-        alert('Error getting data! Please contact our support team by clicking the bottom right link on the page!')


Also should keep this one

I'm bringing the alerts back. Should I remove the rule or ignore them per line?

Good question. Probably ignore on the lines. We don't want to accidentally check in any other alerts and realistically the ONLY time you should use an alert is when everything is failing

ermish · 2025-02-10T02:06:01Z

stores/searchResultsStore.ts

@@ -135,7 +135,7 @@ async function queryFacilities(healthcareProfessionalIds: string[], searchCity?:
        return locationFilteredSearchResults
    } catch (error) {
        console.error(`Error getting facilities: ${JSON.stringify(error)}`)
-        alert('Error getting data! Please contact our support team by clicking the bottom right link on the page!')


And this one

@NabbeunNabi We can make a separate issue for moving these alerts to nicer user modal messages

utils/formValidations.ts

utils/handleScroll.ts

ermish · 2025-02-10T02:10:39Z

utils/handleServerErrorMessaging.ts

@@ -13,7 +13,7 @@ export function handleServerErrorMessaging(
        }
        toast.error((t('serverErrorMessages.genericErrorMessage')))
        console.error((t('serverErrorMessages.genericErrorMessage')))
-        console.info(t('serverErrorMessages.errorCodeMessagingNeeded'))


I think this one should be info since we already have the error right before it

Allow console warn, error and info

The `consistent-return` rule causes inconsistencies across the codebase. It requires returning values like `return undefined`, which is unnecessary.

Renamed `l` and `o` to `currentLocale`

Fix ignores paths

Some lines are supposed to work this way.

gminetoma added code quality vitest Tests related to functional testing with Vitest. labels Feb 9, 2025

gminetoma requested a review from NabbeunNabi February 9, 2025 08:12

gminetoma self-assigned this Feb 9, 2025

gminetoma linked an issue Feb 9, 2025 that may be closed by this pull request

fix: eslint.config.js #982

Open

gminetoma force-pushed the fix/eslint-config branch 2 times, most recently from e3ebf10 to b8ec835 Compare February 9, 2025 08:16

gminetoma marked this pull request as ready for review February 9, 2025 08:21

gminetoma requested review from theyokohamalife and ermish as code owners February 9, 2025 08:21

gminetoma force-pushed the fix/eslint-config branch from b8ec835 to b2d35f0 Compare February 9, 2025 08:27

NabbeunNabi reviewed Feb 9, 2025

View reviewed changes

eslint.config.js Outdated Show resolved Hide resolved

gminetoma force-pushed the fix/eslint-config branch from c276cbb to 5445d95 Compare February 9, 2025 10:17

gminetoma requested a review from NabbeunNabi February 9, 2025 10:19

NabbeunNabi approved these changes Feb 9, 2025

View reviewed changes

NabbeunNabi requested changes Feb 9, 2025

View reviewed changes

gminetoma force-pushed the fix/eslint-config branch from 5445d95 to 1f77280 Compare February 9, 2025 11:16

gminetoma requested a review from NabbeunNabi February 9, 2025 11:20

NabbeunNabi approved these changes Feb 9, 2025

View reviewed changes

ermish requested changes Feb 10, 2025

View reviewed changes

gminetoma added 4 commits February 10, 2025 13:35

fix: eslint.config.js

7edc1c7

feat: add Vitest ESLint plugin

cc7b570

fix: run yarn lint --fix

41875ef

fix: update no-console rule to be more permissive

0d1022c

Allow console warn, error and info

gminetoma force-pushed the fix/eslint-config branch from 1f77280 to 9196c52 Compare February 10, 2025 04:54

gminetoma requested a review from ermish February 10, 2025 04:54

gminetoma force-pushed the fix/eslint-config branch from 9196c52 to b9d41b1 Compare February 10, 2025 04:56

gminetoma added 6 commits February 11, 2025 01:09

fix: no-console

ec1edb4

fix: no-underscore-dangle

9fb19cf

fix: no-shadow

fd82d6e

fix: max-statements-per-line

52def55

refactor: remove consistent-return ESLint rule

af93744

The `consistent-return` rule causes inconsistencies across the codebase. It requires returning values like `return undefined`, which is unnecessary.

refactor: rename variables for better readability

2dfa282

Renamed `l` and `o` to `currentLocale`

gminetoma force-pushed the fix/eslint-config branch from b9d41b1 to ff68d08 Compare February 10, 2025 16:16

gminetoma added 2 commits February 11, 2025 01:20

refactor: ignore checkLocaleKeys.js

bb30e65

Fix ignores paths

refactor: add eslint-disable-next-line no-alert

fbef2f0

Some lines are supposed to work this way.

gminetoma force-pushed the fix/eslint-config branch from ff68d08 to fbef2f0 Compare February 10, 2025 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: ESLint config #1025

fix: ESLint config #1025

gminetoma commented Feb 9, 2025 •

edited

Loading

netlify bot commented Feb 9, 2025 •

edited

Loading

socket-security bot commented Feb 9, 2025 •

edited

Loading

NabbeunNabi left a comment

NabbeunNabi Feb 9, 2025

NabbeunNabi Feb 9, 2025

ermish Feb 10, 2025

NabbeunNabi Feb 9, 2025

gminetoma Feb 9, 2025

ermish Feb 10, 2025

ermish Feb 10, 2025

gminetoma Feb 10, 2025

gminetoma Feb 10, 2025

gminetoma Feb 10, 2025

ermish Feb 10, 2025

gminetoma Feb 10, 2025 •

edited

Loading

NabbeunNabi Feb 9, 2025

ermish Feb 10, 2025

NabbeunNabi left a comment

NabbeunNabi commented Feb 9, 2025

NabbeunNabi left a comment

NabbeunNabi left a comment

ermish left a comment

ermish Feb 10, 2025

gminetoma Feb 10, 2025

ermish Feb 10, 2025

gminetoma Feb 10, 2025 •

edited

Loading

ermish Feb 10, 2025

gminetoma Feb 10, 2025

ermish Feb 10, 2025

gminetoma Feb 10, 2025 •

edited

Loading

ermish Feb 10, 2025 •

edited

Loading

ermish Feb 10, 2025

ermish Feb 10, 2025

ermish Feb 10, 2025

		alert('Error getting data! Please contact our support team by clicking the bottom right link on the page!')
		console.error('Error getting data! Please contact our support team by clicking the bottom right link on the page!')

fix: ESLint config #1025

Are you sure you want to change the base?

fix: ESLint config #1025

Conversation

gminetoma commented Feb 9, 2025 • edited Loading

🔧 What Changed

Added Vitest ESLint Plugin and Fixed the Following Errors:

Styling

no-console

no-underscore-dangle

no-shadow

consistent-return

no-alert

Notes:

netlify bot commented Feb 9, 2025 • edited Loading

✅ Deploy Preview for findadoc ready!

socket-security bot commented Feb 9, 2025 • edited Loading

NabbeunNabi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gminetoma Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NabbeunNabi left a comment

Choose a reason for hiding this comment

NabbeunNabi commented Feb 9, 2025

NabbeunNabi left a comment

Choose a reason for hiding this comment

NabbeunNabi left a comment

Choose a reason for hiding this comment

ermish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gminetoma Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gminetoma Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

ermish Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gminetoma commented Feb 9, 2025 •

edited

Loading

`no-console`

`no-underscore-dangle`

`no-shadow`

`consistent-return`

`no-alert`

netlify bot commented Feb 9, 2025 •

edited

Loading

socket-security bot commented Feb 9, 2025 •

edited

Loading

gminetoma Feb 10, 2025 •

edited

Loading

gminetoma Feb 10, 2025 •

edited

Loading

gminetoma Feb 10, 2025 •

edited

Loading

ermish Feb 10, 2025 •

edited

Loading