chore: Updated eslint configuration #2851

jsumners-nr · 2024-12-23T14:11:40Z

This PR resolve #2852.

This PR updates the eslint configuration, and plugins, to the latest. I discovered that our shared configuration has not kept up-to-date on the packages that it uses. In particular:

eslint released v9 that radically changes (and simplifies) how configuration is written and parsed.
eslint-plugin-node has been out-of-maintenance for a long time, replaced by eslint-plugin-n

The result was a difficult to update shared configuration package. Now that this PR is ready, I could go back and update the shared package to eslint@9 standards, but I'm not convinced the shared package is worth the maintenance cost. Most of the configuration is specific to the repository. I am quite certain that most of the packages we maintain can get by with the simple neostandard configuration.

Having been out-of-date for so long, our code base has developed many "errors" in regard to linting. The sonarjs plugin saw a lot of additions that now trigger errors. So there are many files touched in this PR. I took the approach of overriding the majority of rules for the tests, but using inline comments in the actual library code. I think our tests can be more lenient, and that we can investigate on a case-by-case basis all of the manual overrides.

I think we should wait for the next branch to be merged to main, at which point I will rebase and update this branch, before we merge. But I'm open to merging this first if the team thinks it will not cause too much strife in keeping next synchronized.

codecov · 2024-12-24T15:38:27Z

Codecov Report

Attention: Patch coverage is 97.56098% with 6 lines in your changes missing coverage. Please review.

Project coverage is 97.22%. Comparing base (763c0e6) to head (b21192e).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/transaction/index.js	77.77%	2 Missing ⚠️
lib/instrumentation/core/http-outbound.js	50.00%	1 Missing ⚠️
lib/instrumentation/when/index.js	66.66%	1 Missing ⚠️
lib/transaction/name-state.js	83.33%	1 Missing ⚠️
lib/utilization/common.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2851      +/-   ##
==========================================
- Coverage   97.27%   97.22%   -0.06%     
==========================================
  Files         296      296              
  Lines       46625    46635      +10     
==========================================
- Hits        45354    45340      -14     
- Misses       1271     1295      +24

Flag	Coverage Δ
integration-tests-cjs-18.x	`73.15% <68.93%> (-0.01%)`	⬇️
integration-tests-cjs-20.x	`73.15% <68.93%> (-0.01%)`	⬇️
integration-tests-cjs-22.x	`73.18% <68.93%> (-0.01%)`	⬇️
integration-tests-esm-18.x	`49.90% <41.88%> (-0.03%)`	⬇️
integration-tests-esm-20.x	`49.90% <41.88%> (-0.03%)`	⬇️
integration-tests-esm-22.x	`49.95% <41.88%> (-0.03%)`	⬇️
unit-tests-18.x	`88.90% <91.39%> (-0.01%)`	⬇️
unit-tests-20.x	`88.90% <91.39%> (-0.01%)`	⬇️
unit-tests-22.x	`88.90% <91.39%> (-0.01%)`	⬇️
versioned-tests-18.x	`79.07% <75.51%> (-0.19%)`	⬇️
versioned-tests-20.x	`79.07% <75.51%> (-0.19%)`	⬇️
versioned-tests-22.x	`79.11% <75.51%> (-0.17%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

test/unit/spans/map-to-streaming-type.test.js

eslint-plugin-newrelic-header.js

bizob2828

this is a very large PR, i went through most. you removed all the ignoring of the no console and it won't show this as a warning because lint is quiet but not sure if we want to restore those or not. i also noticed now that lint is quiet there are a few tests that are using t.diagnostic and they should prob be removed

lib/custom-events/custom-event-aggregator.js

lib/db/statement-matcher.js

bizob2828 · 2025-01-02T20:37:35Z

lib/db/utils.js

+
+  // Suppressing a warning on this regex because it is not obvious what this
+  // regex does, and we don't want to break anything.
+  // eslint-disable-next-line sonarjs/slow-regex, sonarjs/duplicates-in-character-class


we should prob log a ticket to fix these slow regex

bizob2828 · 2025-01-02T20:38:31Z

lib/header-attributes.js

@@ -95,6 +95,7 @@ function _headerToCamelCase(header) {
  const newHeader = header.charAt(0).toLowerCase() + header.slice(1)

  // Converts headers in the form 'header-name' to be in the form 'headerName'
+  // eslint-disable-next-line sonarjs/slow-regex
  return newHeader.replace(/[\W_]+(\w)/g, function capitalize(m, $1) {


we should prob log a ticket to fix these slow regex

lib/instrumentation/@prisma/client.js

lib/shim/promise-shim.js

bizob2828 · 2025-01-02T20:57:29Z

lib/util/sql/obfuscate.js

@@ -7,16 +7,19 @@

 module.exports = obfuscate

+// eslint-disable-next-line sonarjs/slow-regex


we should probably log a ticket to address these slow regexs

bizob2828 · 2025-01-02T21:14:39Z

test/unit/api/api-instrument-loaded-module.test.js

@@ -97,9 +97,8 @@ test('Agent API - instrumentLoadedModule', async (t) => {
    const EMPTY_MODULE = {}
    let mod = EMPTY_MODULE
    try {
-      // eslint-disable-next-line node/no-missing-require


i know this switched to eslint-plugin-n but this still should fail, not sure what's going on here

The current configuration applies the eslint-plugin-node "recommended" rules. The configuration in this PR omits them. I experimented with adding them back in yesterday afternoon, but it flagged a whole bunch of things as not supported prior to Node 18/20. It was doing so because it thinks we are trying to target >=16 (the finally fallback as described in https://github.com/eslint-community/eslint-plugin-n?tab=readme-ov-file#configured-nodejs-version-range).

I have implemented some tweaking of the rules for this code base and included it in my latest commit.

OIC, it doesn't define the no-missing-require: https://github.com/neostandard/neostandard/blob/main/lib/configs/base.js#L160

eslint.config.js

jsumners-nr · 2025-01-03T14:02:50Z

you removed all the ignoring of the no console and it won't show this as a warning because lint is quiet but not sure if we want to restore those or not.

Supposedly it was set as "warn" in the original configuration https://github.com/newrelic/eslint-config-newrelic/blob/c5780025fff0c92d6d9012e6132e84fb42ad2459/index.js#L54C1-L54C26

i also noticed now that lint is quiet there are a few tests that are using t.diagnostic and they should prob be removed

Can you clarify this for me? How does the quiet flag affect the t.diagnostic invocations?

I set the main lint script to quiet because we have been ignoring warnings anyway. Do we want the noise to return and just keep overlooking it?

lib/instrumentation/core/http.js

bizob2828 · 2025-01-03T16:28:23Z

you removed all the ignoring of the no console and it won't show this as a warning because lint is quiet but not sure if we want to restore those or not.

Supposedly it was set as "warn" in the original configuration https://github.com/newrelic/eslint-config-newrelic/blob/c5780025fff0c92d6d9012e6132e84fb42ad2459/index.js#L54C1-L54C26

i also noticed now that lint is quiet there are a few tests that are using t.diagnostic and they should prob be removed

Can you clarify this for me? How does the quiet flag affect the t.diagnostic invocations?

I set the main lint script to quiet because we have been ignoring warnings anyway. Do we want the noise to return and just keep overlooking it?

We can address the t.diagnostic in a different PR. but i'm referring to the noise in the PRs now:

bizob2828

I think npm run lint:fix has to be run again. There are a bunch of warnings around object-shorthand that gets fixed by re-running

jsumners-nr · 2025-01-03T17:17:23Z

but i'm referring to the noise in the PRs now:

Ah, okay. Those have been there since we migrated to node:test, but they were hidden by all of the eslint warnings.

bizob2828 · 2025-01-03T17:18:17Z

but i'm referring to the noise in the PRs now:

Ah, okay. Those have been there since we migrated to node:test, but they were hidden by all of the eslint warnings.

Yep, fixed in #2858

bizob2828

LGTM, we can always tweak this as we go

jsumners-nr force-pushed the neostandard branch 6 times, most recently from 2374e99 to 89ff244 Compare December 24, 2024 15:24

jsumners-nr commented Dec 24, 2024

View reviewed changes

test/unit/spans/map-to-streaming-type.test.js Show resolved Hide resolved

jsumners-nr force-pushed the neostandard branch from 89ff244 to af3771d Compare December 24, 2024 17:48

jsumners-nr marked this pull request as ready for review December 24, 2024 18:56

jsumners-nr requested a review from bizob2828 December 24, 2024 18:56

jsumners-nr commented Dec 30, 2024

View reviewed changes

eslint-plugin-newrelic-header.js Outdated Show resolved Hide resolved

jsumners-nr force-pushed the neostandard branch from e7d8868 to 1073db7 Compare January 2, 2025 19:56

chore: Updated eslint configuration

f1eaef1

jsumners-nr force-pushed the neostandard branch from 1073db7 to f1eaef1 Compare January 2, 2025 20:00

bizob2828 requested changes Jan 2, 2025

View reviewed changes

address feedback

b21192e

jsumners-nr requested a review from bizob2828 January 3, 2025 14:02

bizob2828 reviewed Jan 3, 2025

View reviewed changes

lib/instrumentation/core/http.js Outdated Show resolved Hide resolved

bizob2828 requested changes Jan 3, 2025

View reviewed changes

address feedback

26a5827

bizob2828 previously approved these changes Jan 3, 2025

View reviewed changes

remove silly finally blocks

7416815

jsumners-nr dismissed bizob2828’s stale review via 7416815 January 3, 2025 17:24

remove silly self-assign

0900074

jsumners-nr requested a review from bizob2828 January 3, 2025 17:25

bizob2828 previously approved these changes Jan 3, 2025

View reviewed changes

fixup weirdness

6d02567

jsumners-nr dismissed bizob2828’s stale review via 6d02567 January 3, 2025 18:06

bizob2828 approved these changes Jan 3, 2025

View reviewed changes

jsumners-nr merged commit d2fba9d into newrelic:main Jan 3, 2025
25 checks passed

jsumners-nr deleted the neostandard branch January 3, 2025 18:30

jsumners-nr mentioned this pull request Jan 3, 2025

Update shared eslint config and utilize it in this repo #2862

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Updated eslint configuration #2851

chore: Updated eslint configuration #2851

jsumners-nr commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading

bizob2828 left a comment

bizob2828 Jan 2, 2025

jsumners-nr Jan 3, 2025

bizob2828 Jan 2, 2025

jsumners-nr Jan 3, 2025

bizob2828 Jan 2, 2025

jsumners-nr Jan 3, 2025

bizob2828 Jan 2, 2025

jsumners-nr Jan 3, 2025

bizob2828 Jan 3, 2025

jsumners-nr commented Jan 3, 2025

bizob2828 commented Jan 3, 2025

bizob2828 left a comment

jsumners-nr commented Jan 3, 2025

bizob2828 commented Jan 3, 2025

bizob2828 left a comment

		@@ -7,16 +7,19 @@

		module.exports = obfuscate

		// eslint-disable-next-line sonarjs/slow-regex

chore: Updated eslint configuration #2851

chore: Updated eslint configuration #2851

Conversation

jsumners-nr commented Dec 23, 2024 • edited Loading

codecov bot commented Dec 24, 2024 • edited Loading

Codecov Report

bizob2828 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

jsumners-nr commented Jan 3, 2025

bizob2828 commented Jan 3, 2025

bizob2828 left a comment

Choose a reason for hiding this comment

jsumners-nr commented Jan 3, 2025

bizob2828 commented Jan 3, 2025

bizob2828 left a comment

Choose a reason for hiding this comment

jsumners-nr commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 24, 2024 •

edited

Loading