Conversation
b779b41 to
c8b1fd3
Compare
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
c8b1fd3 to
b2f7836
Compare
5b9d895 to
a6afd58
Compare
There was a problem hiding this comment.
Theoretically this is considered a breaking change. Since this is an internal @sentry-internal/* package, we can release this in a non-breaking fashion
Before we merge this, let's check in with Electron, Lynx and React-Native SDK maintainers. They all use a 10.x version of @sentry-internal/eslint-config-sdk so we should make sure this change doesn't cause problems for them:
I agree btw that we shouldn't have to stick to semver for this internal package. Let's just make sure we break no-one unexpectedly 😅
b2ce5c4 to
de82cc5
Compare
|
@Lms24 I just asked internally and it seems to be ok for the others |
bbc1816 to
e6cd95c
Compare
Lms24
left a comment
There was a problem hiding this comment.
Nice! Had some clarifying questions but nothing blocking. Thanks for taking care of this!
| ); | ||
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises |
There was a problem hiding this comment.
why is this rule no longer applied? (no objections here since it's a test but we still want this rule in src files)
There was a problem hiding this comment.
It is a typescript eslint rule and it was never working, since it was a .js file. We didn't configure that plugin for JavaScript files
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable import/export */ | |||
There was a problem hiding this comment.
l: can we add a small explanation why we disable this here and in the other files? I guess once the false positive gets fixed, we'd get an "unused directive" lint error, so whoever runs into this will have some context.
There was a problem hiding this comment.
Good idea. I'll add a comment everywhere directly
| winterCGHeadersToDict, | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| anrIntegration, | ||
| // eslint-disable-next-line deprecation/deprecation |
There was a problem hiding this comment.
Hmm I guess it doesn't flag the deprecation here anymore because only some signatures of this API are deprecated 🤔
I think that's probably fine, was just curious if you think there might be another reason.
sentry-javascript/packages/node-core/src/integrations/anr/index.ts
Lines 268 to 279 in a1cc675
There was a problem hiding this comment.
Very good point - I'll investigate into that one
There was a problem hiding this comment.
Quite interesting - atm TypeScript does actually not inherit the jsdoc with the deprecated warning: microsoft/TypeScript#407
So it could be that the eslint plugin is now more like TypeScripts behavior and ignores that. I readded that: df47728 (#18264)
| /* eslint-disable jsdoc/require-jsdoc */ | ||
| /* eslint-disable max-lines */ | ||
| /* eslint-disable no-param-reassign */ | ||
| /* eslint-disable import/named */ |
There was a problem hiding this comment.
l: why do we need this new disable directive?
There was a problem hiding this comment.
same as here there seems to be a problem with opentelemetry. I added it ad the top, since this one is vendored in
| // these somehow fail with rollup.examples.config.mjs | ||
| files: ['*.mjs'], | ||
| rules: { | ||
| 'import/named': 'off', |
2819171 to
2cdbe7c
Compare
Lms24
left a comment
There was a problem hiding this comment.
thx for replying to my suggestions/questions!

This upgrades our internal eslint config to use v8. Theoretically this is considered a breaking change. Since this is an internal
@sentry-internal/*package, we can release this in a non-breaking fashionWith v8 couple of changes came. Some of them are affecting us, which required file changes:
Merge checks