Enable recommended rules for eslint-plugin-n #5216

watson · 2025-02-06T10:46:31Z

What does this PR do?

Enables all recommended rules from the eslint-plugin-n ESLint plugin that could reasonably be enabled without too much work. The following rules have for now been disabled as they caused too many errors when enabled:

n/hashbang
n/no-process-exit
n/no-missing-require (only disabled in tests)

For now all eslint-plugin-n rules have also been disabled for **/*.mjs files as these for some reason resulting in parsing errors.

Motivation

Plugin Checklist

Additional Notes

watson · 2025-02-06T10:46:49Z

Enable recommended rules for eslint-plugin-n #5216 : 2 dependent PRs (#5218 , #5274 ) 👈 (View in Graphite)
Upgrade ESLint from v8 to v9 #5215
Clean up ESLint config #5214
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-02-06T10:47:52Z

Overall package size

Self size: 8.69 MB
Deduped: 94.88 MB
No deduping: 95.4 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.5.1 | 9.79 MB | 10.17 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 835.4 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2025-02-06T10:48:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.24%. Comparing base (6b97186) to head (1d97adc).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5216   +/-   ##
=======================================
  Coverage   81.24%   81.24%           
=======================================
  Files         487      487           
  Lines       21703    21703           
=======================================
  Hits        17633    17633           
  Misses       4070     4070

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

datadog-datadog-prod-us1 · 2025-02-06T10:52:41Z

Datadog Report

Branch report: watson/enable-eslint-plugin-n
Commit report: 643e416
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 629 Passed, 0 Skipped, 15m 51.08s Total Time

pr-commenter · 2025-02-06T10:57:23Z

Benchmarks

Benchmark execution time: 2025-02-14 12:32:09

Comparing candidate commit 1d97adc in PR branch watson/enable-eslint-plugin-n with baseline commit 6b97186 in branch master.

Found 0 performance improvements and 3 performance regressions! Performance is the same for 905 metrics, 25 unstable metrics.

scenario:plugin-graphql-with-async-hooks-18

🟥 cpu_user_time [+245.240ms; +277.732ms] or [+5.542%; +6.276%]
🟥 execution_time [+304.006ms; +335.104ms] or [+6.316%; +6.962%]
🟥 max_rss_usage [+90.971MB; +94.613MB] or [+17.220%; +17.909%]

benchmark/sirun/plugin-bluebird/index.js

watson · 2025-02-14T12:08:37Z

index.js

+// TODO: It shouldn't be necessary to disable n/no-unpublished-require - Research
+// eslint-disable-next-line n/no-unpublished-require


Will be fixed in #5274

watson · 2025-02-14T12:08:43Z

init.js

+// TODO: It shouldn't be necessary to disable n/no-unpublished-require - Research
+// eslint-disable-next-line n/no-unpublished-require


Will be fixed in #5274

watson · 2025-02-14T12:08:55Z

esbuild.js

+// TODO: It shouldn't be necessary to disable n/no-unpublished-require - Research
+// eslint-disable-next-line n/no-unpublished-require


Will be fixed in #5274

watson · 2025-02-14T12:11:07Z

package.json

@@ -157,6 +157,7 @@
    "sinon-chai": "^3.7.0",
    "tap": "^16.3.7",
    "tiktoken": "^1.0.15",
+    "workerpool": "^9.2.0",


Our code worked previously because workerpool just happened to be a sub-dependency of another one of our dependencies. That sub-dependency was v6.5.1 though, so some breaking changes has been introduced between that and this. For an overview of them, see https://github.com/josdejong/workerpool/blob/master/HISTORY.md

@juan-fernandez The workerpool module is used by CI Visibility in integration-tests/ci-visibility/run-workerpool.js, so you should probably take a look to check that everything is ok. However, I assume it is, since the integration tests are passing after the upgrade.

tlhunter · 2025-02-14T16:56:19Z

packages/datadog-plugin-fetch/test/integration-test/server.mjs

@@ -1,3 +1,5 @@
+/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['fetch'] }] */


The docs are a little confusing. It seems to suggest that the engines field of package.json is used to check node compat (in our case v18). But those same docs also seem to suggest this plugin is stuck in the world of 13.2.0?

https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unsupported-features/node-builtins.md

Overall it seems bad that we need to add such a line to the top of files to use a built in global. Like we'll make a change, push it, CI fails, add the rule, push again.

I agree it's easy to be confused. We are not using eslint-plugin-node, but eslint-plugin-n, which exists exactly because eslint-plugin-node is no longer maintained. The right docs for this is:

https://github.com/eslint-community/eslint-plugin-n/blob/master/docs/rules/no-unsupported-features/node-builtins.md

There is a few places we use features newer than v18.0.0, but we feature detect, e.g. if (globalThis.fetch) { ... }. The ESLint plugin is not clever enough to detect this and complains, but I think that's better than we accidentally used that API without the surrounding if-block.

There's also a few places in tests where we need to add these ESLint comments. And you could argue we might just turn this plugin off in tests 🤷 Or alternatively configure it to be the latest v18.x in tests instead of v18.0.0 as it is in the rest of our published source. That way we could get a rid of a few of these comments for those locations. Do you think that would be worth while, is it good enough as it is?

watson self-assigned this Feb 6, 2025

watson mentioned this pull request Feb 6, 2025

Clean up ESLint config #5214

Merged

6 tasks

watson mentioned this pull request Feb 6, 2025

Upgrade ESLint from v8 to v9 #5215

Merged

6 tasks

watson added the semver-patch label Feb 6, 2025

watson force-pushed the watson/enable-eslint-plugin-n branch 2 times, most recently from a00cbf6 to c0412e5 Compare February 6, 2025 11:22

watson changed the base branch from watson/clean-eslint-config to watson/bump-eslint February 6, 2025 11:22

watson mentioned this pull request Feb 6, 2025

ESLint: Require 'use strict' header #5218

Draft

6 tasks

watson force-pushed the watson/bump-eslint branch from 384864d to 664c0fc Compare February 10, 2025 13:08

watson force-pushed the watson/enable-eslint-plugin-n branch from c0412e5 to b29b51c Compare February 10, 2025 13:08

Base automatically changed from watson/bump-eslint to master February 11, 2025 13:05

watson force-pushed the watson/enable-eslint-plugin-n branch from b29b51c to 451520a Compare February 11, 2025 13:33

watson marked this pull request as ready for review February 11, 2025 13:34

watson requested review from a team as code owners February 11, 2025 13:34

watson requested review from liashenko and bengl February 11, 2025 13:34

rochdev reviewed Feb 11, 2025

View reviewed changes

benchmark/sirun/plugin-bluebird/index.js Outdated Show resolved Hide resolved

watson requested a review from rochdev February 11, 2025 14:32

watson force-pushed the watson/enable-eslint-plugin-n branch from f012b27 to 286701a Compare February 13, 2025 13:31

watson added 2 commits February 14, 2025 12:24

Enable recommended rules for eslint-plugin-n

4d8fd09

Disable n/no-missing-require in benchmark folder

a08679a

Fix subproject warnings

74bc477

watson force-pushed the watson/enable-eslint-plugin-n branch from 286701a to 74bc477 Compare February 14, 2025 11:25

watson mentioned this pull request Feb 14, 2025

Use files property in package.json instead of .npmignore #5274

Draft

6 tasks

watson commented Feb 14, 2025

View reviewed changes

Fix missing workerpool dev-dependency

1d97adc

watson force-pushed the watson/enable-eslint-plugin-n branch from 6c6e8b4 to 1d97adc Compare February 14, 2025 12:21

watson requested review from juan-fernandez and a team February 14, 2025 12:34

tlhunter reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable recommended rules for eslint-plugin-n #5216

Enable recommended rules for eslint-plugin-n #5216

watson commented Feb 6, 2025 •

edited

Loading

watson commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Feb 6, 2025 •

edited

Loading

pr-commenter bot commented Feb 6, 2025 •

edited

Loading

watson Feb 14, 2025

watson Feb 14, 2025

watson Feb 14, 2025

watson Feb 14, 2025

watson Feb 14, 2025

tlhunter Feb 14, 2025

watson Feb 14, 2025 •

edited

Loading

		// TODO: It shouldn't be necessary to disable n/no-unpublished-require - Research
		// eslint-disable-next-line n/no-unpublished-require

		@@ -1,3 +1,5 @@
		/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['fetch'] }] */

Enable recommended rules for eslint-plugin-n #5216

Are you sure you want to change the base?

Enable recommended rules for eslint-plugin-n #5216

Conversation

watson commented Feb 6, 2025 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

watson commented Feb 6, 2025 • edited Loading

github-actions bot commented Feb 6, 2025 • edited Loading

Overall package size

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

datadog-datadog-prod-us1 bot commented Feb 6, 2025 • edited Loading

Datadog Report

pr-commenter bot commented Feb 6, 2025 • edited Loading

Benchmarks

scenario:plugin-graphql-with-async-hooks-18

watson Feb 14, 2025

Choose a reason for hiding this comment

watson Feb 14, 2025

Choose a reason for hiding this comment

watson Feb 14, 2025

Choose a reason for hiding this comment

watson Feb 14, 2025

Choose a reason for hiding this comment

watson Feb 14, 2025

Choose a reason for hiding this comment

tlhunter Feb 14, 2025

Choose a reason for hiding this comment

watson Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

watson commented Feb 6, 2025 •

edited

Loading

watson commented Feb 6, 2025 •

edited

Loading

github-actions bot commented Feb 6, 2025 •

edited

Loading

codecov bot commented Feb 6, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Feb 6, 2025 •

edited

Loading

pr-commenter bot commented Feb 6, 2025 •

edited

Loading

watson Feb 14, 2025 •

edited

Loading