-
Notifications
You must be signed in to change notification settings - Fork 306
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.
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
modernize eslint config #4759
base: master
Are you sure you want to change the base?
modernize eslint config #4759
Conversation
} else { | ||
spanContext = this._extractB3MultiContext(carrier) | ||
} | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khanayan123 You added the original lines here, so can you verify that this logic is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right to me
@@ -0,0 +1 @@ | |||
library_entrypoint {"metadata":{"language_name":"nodejs","language_version":"22.2.0","runtime_name":"nodejs","runtime_version":"22.2.0","tracer_version":"6.0.0-pre","pid":38665},"points":[{"name":"library_entrypoint.complete","tags":["injection_forced:false"]}]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope. will remove.
74303e3
to
17a7ad2
Compare
Overall package sizeSelf size: 7.48 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.1.1 | 18.67 MB | 18.68 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.59 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | jsonpath-plus | 10.0.0 | 659.84 kB | 1.12 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | lru-cache | 7.14.0 | 74.95 kB | 74.95 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 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 | | 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 | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 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 |
17a7ad2
to
00c602e
Compare
00c602e
to
d049b1e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it! Thanks for doing this
Besides the review comments that I just left, I think it would be a good idea if we sorted the rules in the rules
objects alphabetically. That way all rules from for example mocha
or import
would all be grouped together and there would never be any doubt where a new rule should be added.
eslint.config.mjs
Outdated
'**/vendor', | ||
'integration-tests/esbuild/out.js', | ||
'integration-tests/esbuild/aws-sdk-out.js', | ||
'packages/dd-trace/src/appsec/blocked_templates.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ignore list is pulled from the previous .eslintignore
file. I'm deliberately not changing the ignore list in order to minimize impact of this change.
I'll add a TODO for adding comments explaining why each one is ignored.
eslint.config.mjs
Outdated
'max-len': [2, 120, 2], | ||
'no-var': 2, | ||
'no-console': 2, | ||
'prefer-const': 2, | ||
'object-curly-spacing': [2, 'always'], | ||
'import/no-extraneous-dependencies': 2, | ||
'standard/no-callback-literal': 0, | ||
'no-prototype-builtins': 0, | ||
'n/no-restricted-require': [2, ['diagnostics_channel']], | ||
'n/no-callback-literal': 0, | ||
|
||
'object-curly-newline': ['error', { | ||
multiline: true, | ||
consistent: true | ||
}], | ||
|
||
'import/no-absolute-path': 0, | ||
'no-unused-expressions': 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes the file much more readable if you use the string literals for off
, warn
, and error
instead of the numbers 0-2. So this block would be:
'max-len': [2, 120, 2], | |
'no-var': 2, | |
'no-console': 2, | |
'prefer-const': 2, | |
'object-curly-spacing': [2, 'always'], | |
'import/no-extraneous-dependencies': 2, | |
'standard/no-callback-literal': 0, | |
'no-prototype-builtins': 0, | |
'n/no-restricted-require': [2, ['diagnostics_channel']], | |
'n/no-callback-literal': 0, | |
'object-curly-newline': ['error', { | |
multiline: true, | |
consistent: true | |
}], | |
'import/no-absolute-path': 0, | |
'no-unused-expressions': 0 | |
'max-len': ['error', 120, 2], | |
'no-var': 'error', | |
'no-console': 'error', | |
'prefer-const': 'error', | |
'object-curly-spacing': ['error', 'always'], | |
'import/no-extraneous-dependencies': 'error', | |
'standard/no-callback-literal': 'off', | |
'no-prototype-builtins': 'off', | |
'n/no-restricted-require': ['error', ['diagnostics_channel']], | |
'n/no-callback-literal': 'off', | |
'object-curly-newline': ['error', { | |
multiline: true, | |
consistent: true | |
}], | |
'import/no-absolute-path': 'off', | |
'no-unused-expressions': 'off' |
And then the equivalent with the other rules
objects below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@watson do you know of a plugin or rule that would enforce this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha I haven't checked if one exists, but I wouldn't be surprised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did some quick googling today and I haven't been able find anything 🙃
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4759 +/- ##
==========================================
- Coverage 91.42% 83.93% -7.49%
==========================================
Files 112 292 +180
Lines 3475 12078 +8603
Branches 33 33
==========================================
+ Hits 3177 10138 +6961
- Misses 298 1940 +1642
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed a few updates to to the max-len
rule rename in integration-tests/debugger/index.spec.js
- most likely because they are in a file that was just merged into master
and your local checked out version hasn't been updated to include this file.
eslint.config.mjs
Outdated
'@stylistic/js/max-len': ['error', { code: 120, tabWidth: 2 }], | ||
'no-var': 'error', | ||
'no-console': 'error', | ||
'prefer-const': 'error', | ||
'@stylistic/js/object-curly-spacing': ['error', 'always'], | ||
'import/no-extraneous-dependencies': 'error', | ||
'standard/no-callback-literal': 'off', | ||
'no-prototype-builtins': 'off', | ||
'n/no-restricted-require': ['error', ['diagnostics_channel']], | ||
'n/no-callback-literal': 'off', | ||
|
||
'@stylistic/js/object-curly-newline': ['error', { | ||
multiline: true, | ||
consistent: true | ||
}], | ||
|
||
'import/no-absolute-path': 'off', | ||
'no-unused-expressions': 'off' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I still prefer that we sorted these:
'@stylistic/js/max-len': ['error', { code: 120, tabWidth: 2 }], | |
'no-var': 'error', | |
'no-console': 'error', | |
'prefer-const': 'error', | |
'@stylistic/js/object-curly-spacing': ['error', 'always'], | |
'import/no-extraneous-dependencies': 'error', | |
'standard/no-callback-literal': 'off', | |
'no-prototype-builtins': 'off', | |
'n/no-restricted-require': ['error', ['diagnostics_channel']], | |
'n/no-callback-literal': 'off', | |
'@stylistic/js/object-curly-newline': ['error', { | |
multiline: true, | |
consistent: true | |
}], | |
'import/no-absolute-path': 'off', | |
'no-unused-expressions': 'off' | |
'@stylistic/js/max-len': ['error', { code: 120, tabWidth: 2 }], | |
'@stylistic/js/object-curly-newline': ['error', { multiline: true, consistent: true }], | |
'@stylistic/js/object-curly-spacing': ['error', 'always'], | |
'import/no-absolute-path': 'off', | |
'import/no-extraneous-dependencies': 'error', | |
'n/no-callback-literal': 'off', | |
'n/no-restricted-require': ['error', ['diagnostics_channel']], | |
'no-console': 'error', | |
'no-prototype-builtins': 'off', | |
'no-unused-expressions': 'off', | |
'no-var': 'error', | |
'prefer-const': 'error', | |
'standard/no-callback-literal': 'off' |
eslint.config.mjs
Outdated
'mocha/no-mocha-arrows': 'off', | ||
'mocha/no-setup-in-describe': 'off', | ||
'mocha/no-sibling-hooks': 'off', | ||
'mocha/no-top-level-hooks': 'off', | ||
'mocha/max-top-level-suites': 'off', | ||
'mocha/no-identical-title': 'off', | ||
'mocha/no-global-tests': 'off', | ||
'mocha/no-exports': 'off', | ||
'mocha/no-skipped-tests': 'off', | ||
'n/handle-callback-err': 'off', | ||
'no-loss-of-precision': 'off' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: same here:
'mocha/no-mocha-arrows': 'off', | |
'mocha/no-setup-in-describe': 'off', | |
'mocha/no-sibling-hooks': 'off', | |
'mocha/no-top-level-hooks': 'off', | |
'mocha/max-top-level-suites': 'off', | |
'mocha/no-identical-title': 'off', | |
'mocha/no-global-tests': 'off', | |
'mocha/no-exports': 'off', | |
'mocha/no-skipped-tests': 'off', | |
'n/handle-callback-err': 'off', | |
'no-loss-of-precision': 'off' | |
'mocha/max-top-level-suites': 'off', | |
'mocha/no-exports': 'off', | |
'mocha/no-global-tests': 'off', | |
'mocha/no-identical-title': 'off', | |
'mocha/no-mocha-arrows': 'off', | |
'mocha/no-setup-in-describe': 'off', | |
'mocha/no-sibling-hooks': 'off', | |
'mocha/no-skipped-tests': 'off', | |
'mocha/no-top-level-hooks': 'off', | |
'n/handle-callback-err': 'off', | |
'no-loss-of-precision': 'off' |
* Switch from the old eslintrc format to the newer format via: `npx @eslint/migrate-config .eslintrc.json` * ECMAScript version is now set at 2022, in line with code supported in Node.js 16. This is needed for a bunch of ESM syntax like top-level await. * Fixes: * ESM files are now covered. * Test globals and other test-specific config are now isolated to tests. * text_map.js has an invalid switch case. Fixed that in what I thought was the most reasonable way.
0e67eb0
to
3729bbd
Compare
What does this PR do?
npx @eslint/migrate-config .eslintrc.json
Motivation
I was trying to add new local lint rules, but that's only supported by the modern config format. Along the way I noticed we weren't covering ESM files at all.