Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Enable recommended rules for eslint-plugin-n #5216

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Enable recommended rules for eslint-plugin-n #5216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb8ea64
Enable recommended rules for eslint-plugin-n
watson Feb 6, 2025
1378803
Disable n/no-missing-require in benchmark folder
watson Feb 11, 2025
b286556
Fix subproject warnings
watson Feb 14, 2025
cc489e5
Fix missing workerpool dev-dependency
watson Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions LICENSE-3rdparty.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ dev,sinon,BSD-3-Clause,Copyright 2010-2017 Christian Johansen
dev,sinon-chai,WTFPL and BSD-2-Clause,Copyright 2004 Sam Hocevar 2012–2017 Domenic Denicola
dev,tap,ISC,Copyright 2011-2022 Isaac Z. Schlueter and Contributors
dev,tiktoken,MIT,Copyright (c) 2022 OpenAI, Shantanu Jain
dev,workerpool,Apache license 2.0,Copyright (C) 2014-2024 Jos de Jong [email protected]
dev,yaml,ISC,Copyright Eemeli Aro <[email protected]>
file,aws-lambda-nodejs-runtime-interface-client,Apache 2.0,Copyright 2019 Amazon.com Inc. or its affiliates. All Rights Reserved.
file,profile.proto,Apache license 2.0,Copyright 2016 Google Inc.
Expand Down
2 changes: 2 additions & 0 deletions esbuild.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict'

// TODO: It shouldn't be necessary to disable n/no-unpublished-require - Research
// eslint-disable-next-line n/no-unpublished-require
Comment on lines +3 to +4
Copy link
Collaborator Author

@watson watson Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Will be fixed in #5320

module.exports = require('./packages/datadog-esbuild/index.js')
31 changes: 30 additions & 1 deletion eslint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,22 @@ export default [
},
{ name: '@eslint/js/recommnded', ...eslintPluginJs.configs.recommended },
...compat.extends('standard').map((config, i) => ({ name: config.name || `standard/${i + 1}`, ...config })),
{
...n.configs['flat/recommended'],
ignores: [
'packages/dd-trace/test/appsec/next/app-dir/**/*.js',
'packages/dd-trace/test/appsec/next/pages-dir/**/*.js',
'packages/datadog-plugin-next/test/app/**/*.js',
'packages/datadog-plugin-next/test/**/pages/**/*.js',
'packages/datadog-plugin-next/test/middleware.js',
'**/*.mjs' // TODO: This shoudln't be required, research why it is
]
},
{
name: 'dd-trace/defaults',

plugins: {
'@stylistic/js': eslintPluginStylistic,
n: eslintPluginN,
unicorn: eslintPluginUnicorn
},

Expand Down Expand Up @@ -84,6 +94,15 @@ export default [
'@stylistic/js/object-curly-spacing': ['error', 'always'],
'import/no-extraneous-dependencies': 'error',
'n/no-restricted-require': ['error', ['diagnostics_channel']],
'n/hashbang': 'off', // TODO: Enable this rule once we have a plan to address it
'n/no-process-exit': 'off', // TODO: Enable this rule once we have a plan to address it
'n/no-unsupported-features/node-builtins': ['error', {
ignores: [
'async_hooks.createHook',
'async_hooks.executionAsyncId',
'async_hooks.executionAsyncResource'
]
}],
'no-console': 'error',
'no-prototype-builtins': 'off', // Override (turned on by @eslint/js/recommnded)
'no-unused-expressions': 'off', // Override (turned on by standard)
Expand Down Expand Up @@ -189,6 +208,15 @@ export default [
...eslintPluginMocha.configs.flat.recommended,
files: TEST_FILES
},
{
name: 'dd-trace/benchmarks',
files: [
'benchmark/**/*'
],
rules: {
'n/no-missing-require': 'off'
}
},
{
name: 'dd-trace/tests/all',
files: TEST_FILES,
Expand All @@ -214,6 +242,7 @@ export default [
'mocha/no-skipped-tests': 'off',
'mocha/no-top-level-hooks': 'off',
'n/handle-callback-err': 'off',
'n/no-missing-require': 'off',
'require-await': 'off'
}
},
Expand Down
2 changes: 2 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict'

// TODO: It shouldn't be necessary to disable n/no-unpublished-require - Research
// eslint-disable-next-line n/no-unpublished-require
Comment on lines +3 to +4
Copy link
Collaborator Author

@watson watson Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Will be fixed in #5320

module.exports = require('./packages/dd-trace')
2 changes: 2 additions & 0 deletions init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

/* eslint-disable no-var */

// TODO: It shouldn't be necessary to disable n/no-unpublished-require - Research
// eslint-disable-next-line n/no-unpublished-require
Comment on lines +5 to +6
Copy link
Collaborator Author

@watson watson Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Will be fixed in #5320

var guard = require('./packages/dd-trace/src/guardrails')

module.exports = guard(function () {
Expand Down
2 changes: 2 additions & 0 deletions initialize.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
* hook will always be active for ESM support.
*/

/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['module.register'] }] */

import { isMainThread } from 'worker_threads'

import * as Module from 'node:module'
Expand Down
1 change: 1 addition & 0 deletions integration-tests/appsec/esm-app/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['module.register'] }] */

import childProcess from 'node:child_process'
import express from 'express'
Expand Down
1 change: 1 addition & 0 deletions integration-tests/appsec/multer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { allowExperimental: true }] */

const { assert } = require('chai')
const path = require('path')
Expand Down
5 changes: 4 additions & 1 deletion integration-tests/ci-visibility/subproject/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,8 @@
"name": "subproject",
"private": true,
"version": "1.0.0",
"description": "app within repo"
"description": "app within repo",
"dependencies": {
"dd-trace": "file:../../.."
}
}
1 change: 0 additions & 1 deletion integration-tests/ci-visibility/subproject/subproject-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
const { expect } = require('chai')
const dependency = require('./dependency')

describe('subproject-test', () => {
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@
"sinon-chai": "^3.7.0",
"tap": "^16.3.7",
"tiktoken": "^1.0.15",
"workerpool": "^9.2.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

"yaml": "^2.5.0"
}
}
1 change: 1 addition & 0 deletions packages/datadog-instrumentations/src/fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['fetch', 'Request'] }] */

const { isInServerlessEnvironment } = require('../../dd-trace/src/serverless')

Expand Down
1 change: 1 addition & 0 deletions packages/datadog-instrumentations/test/multer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['FormData'] }] */

const dc = require('dc-polyfill')
const axios = require('axios')
Expand Down
1 change: 1 addition & 0 deletions packages/datadog-plugin-fetch/test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['fetch', 'Request'] }] */

const agent = require('../../dd-trace/test/plugins/agent')
const tags = require('../../../ext/tags')
Expand Down
1 change: 1 addition & 0 deletions packages/datadog-plugin-fetch/test/integration-test/client.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
} = require('../../../../integration-tests/helpers')
const { assert } = require('chai')

// eslint-disable-next-line n/no-unsupported-features/node-builtins
const describe = globalThis.fetch ? globalThis.describe : globalThis.describe.skip

describe('esm', () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/datadog-plugin-fetch/test/integration-test/server.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['fetch'] }] */
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@watson watson Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess limiting the linter for everything unrelated to tests would be good


import 'dd-trace/init.js'
import getPort from 'get-port'

Expand Down
1 change: 1 addition & 0 deletions packages/dd-trace/src/appsec/iast/taint-tracking/rewriter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['module.register'] }] */

const Module = require('module')
const { pathToFileURL } = require('url')
Expand Down
1 change: 1 addition & 0 deletions packages/dd-trace/src/debugger/devtools_client/inspector_promises_polyfill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['inspector/promises'] }] */

const { builtinModules } = require('node:module')

Expand Down
3 changes: 3 additions & 0 deletions packages/dd-trace/src/profiling/exporters/event_serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['os.availableParallelism'] }] */

const os = require('os')
const perf = require('perf_hooks').performance
const version = require('../../../../../package.json').version
Expand Down
1 change: 1 addition & 0 deletions packages/dd-trace/src/runtime_metrics/runtime_metrics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['v8.GCProfiler'] }] */

// TODO: capture every second and flush every 10 seconds

Expand Down
1 change: 1 addition & 0 deletions packages/dd-trace/test/appsec/iast/analyzers/path-traversal-analyzer.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['fs.cp'] }] */

const os = require('os')
const path = require('path')
Expand Down
1 change: 1 addition & 0 deletions packages/dd-trace/test/appsec/iast/resources/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['vm.SourceTextModule'] }] */

const tracer = require('dd-trace')
tracer.init({
Expand Down
1 change: 1 addition & 0 deletions packages/dd-trace/test/appsec/index.next.plugin.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict'
/* eslint n/no-unsupported-features/node-builtins: ['error', { ignores: ['fs.cpSync'] }] */

const { spawn, execSync } = require('child_process')
const { cpSync, mkdirSync, rmdirSync, unlinkSync } = require('fs')
Expand Down
2 changes: 2 additions & 0 deletions register.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* eslint n/no-unsupported-features/node-builtins: ['error', { version: '>=20.6.0', allowExperimental: true }] */

const { register } = require('node:module')
const { pathToFileURL } = require('node:url')

Expand Down
2 changes: 2 additions & 0 deletions scripts/get-chrome-driver-download-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
/* eslint n/no-unsupported-features/node-builtins: ['error', { version: '>=21.0.0', allowExperimental: true }] */

const URL = 'https://googlechromelabs.github.io/chrome-for-testing/known-good-versions-with-downloads.json'

// Get chrome driver download URL from a given chrome version, provided via CHROME_VERSION env var
Expand Down
1 change: 1 addition & 0 deletions scripts/verify-ci-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict'
/* eslint-disable no-console */
/* eslint n/no-unsupported-features/node-builtins: ['error', { version: '>=22.0.0', allowExperimental: true }] */

const fs = require('fs')
const path = require('path')
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5395,6 +5395,11 @@ workerpool@^6.5.1:
resolved "https://registry.yarnpkg.com/workerpool/-/workerpool-6.5.1.tgz#060f73b39d0caf97c6db64da004cd01b4c099544"
integrity sha512-Fs4dNYcsdpYSAfVxhnl1L5zTksjvOJxtC5hzMNl+1t9B8hTJTdKDyZ5ju7ztgPy+ft9tBFXoOlDNiOT9WUXZlA==

workerpool@^9.2.0:
version "9.2.0"
resolved "https://registry.yarnpkg.com/workerpool/-/workerpool-9.2.0.tgz#f74427cbb61234708332ed8ab9cbf56dcb1c4371"
integrity sha512-PKZqBOCo6CYkVOwAxWxQaSF2Fvb5Iv2fCeTP7buyWI2GiynWr46NcXSgK/idoV6e60dgCBfgYc+Un3HMvmqP8w==

wrap-ansi@^6.2.0:
version "6.2.0"
resolved "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-6.2.0.tgz"
Expand Down
Loading