ref(types): Avoid some `any` type casting around `wrap` code #14463

mydea · 2024-11-25T15:31:00Z

We use a heavy dose of any, that I would like to reduce.

So I started out to look into wrap(), which made heave use of this, and tried to rewrite it to avoid any as much as possible. This required some changes around it, but should now have much better type inferrence etc. than before, and be more "realistic" in what it tells you.

While at this, I also removed the before argument that we were not using anymore - wrap is not exported anymore, so this is purely internal.

github-actions · 2024-11-25T15:42:30Z

size-limit report 📦

Path	Size	% Change	Change
@sentry/browser	23.12 KB	-0.1%	-22 B 🔽
@sentry/browser - with treeshaking flags	21.82 KB	-0.2%	-44 B 🔽
@sentry/browser (incl. Tracing)	35.53 KB	-0.07%	-22 B 🔽
@sentry/browser (incl. Tracing, Replay)	72.44 KB	-0.03%	-22 B 🔽
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags	62.81 KB	-0.05%	-31 B 🔽
@sentry/browser (incl. Tracing, Replay with Canvas)	76.76 KB	-0.03%	-21 B 🔽
@sentry/browser (incl. Tracing, Replay, Feedback)	89.22 KB	-0.02%	-15 B 🔽
@sentry/browser (incl. Feedback)	39.86 KB	-0.04%	-13 B 🔽
@sentry/browser (incl. sendFeedback)	27.74 KB	-0.08%	-20 B 🔽
@sentry/browser (incl. FeedbackAsync)	32.54 KB	-0.06%	-19 B 🔽
@sentry/react	25.79 KB	-0.08%	-20 B 🔽
@sentry/react (incl. Tracing)	38.37 KB	-0.06%	-20 B 🔽
@sentry/vue	27.27 KB	-0.08%	-22 B 🔽
@sentry/vue (incl. Tracing)	37.33 KB	-0.06%	-22 B 🔽
@sentry/svelte	23.27 KB	-0.09%	-20 B 🔽
CDN Bundle	24.31 KB	-0.08%	-19 B 🔽
CDN Bundle (incl. Tracing)	37.19 KB	-0.07%	-24 B 🔽
CDN Bundle (incl. Tracing, Replay)	72.06 KB	-0.04%	-25 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback)	77.4 KB	-0.03%	-23 B 🔽
CDN Bundle - uncompressed	71.42 KB	-0.06%	-37 B 🔽
CDN Bundle (incl. Tracing) - uncompressed	110.47 KB	-0.04%	-37 B 🔽
CDN Bundle (incl. Tracing, Replay) - uncompressed	223.54 KB	-0.02%	-37 B 🔽
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed	236.76 KB	-0.02%	-37 B 🔽
@sentry/nextjs (client)	38.69 KB	-0.08%	-31 B 🔽
@sentry/sveltekit (client)	36.05 KB	-0.08%	-28 B 🔽
@sentry/node	135.23 KB	-	-
@sentry/node - without tracing	97.04 KB	-0.01%	-1 B 🔽
@sentry/aws-serverless	107.29 KB	-	-

View base workflow run

codecov · 2024-11-25T15:44:52Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
364	1	363	70

View the full list of 1 ❄️ flaky tests

integrations/ContextLines/noAddedLines/test.ts should not add source context lines to errors from script files

Flake rate in main: 11.52% (Passed 215 times, Failed 28 times)

Stack Traces | 30.1s run time

test.ts:6:11 should not add source context lines to errors from script files

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

mydea · 2024-11-25T16:02:31Z

packages/browser/src/helpers.ts

@@ -31,6 +31,21 @@ export function ignoreNextOnError(): void {
  });
 }

+// eslint-disable-next-line @typescript-eslint/ban-types
+type WrappableFunction = Function;


Just an alias to avoid having ban-types eslint disable everywhere

mydea · 2024-11-25T16:03:03Z

packages/browser/src/helpers.ts

+// eslint-disable-next-line @typescript-eslint/ban-types
+type WrappableFunction = Function;
+
+export function wrap<T extends WrappableFunction>(


This ensures we have proper type inferrence, so e.g.

const a = wrap(1); // a is correctly inferred as `1` now

mydea · 2024-11-25T16:04:31Z

packages/browser/src/integrations/browserapierrors.ts


-  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-prototype-builtins
-  if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) {
+  if (!proto || Object.prototype.hasOwnProperty.call(proto, 'addEventListener')) {


is there a reason not to use Object.prototype.hasOwnProperty here, but use the prototype method itself? 🤔

what's the difference between the two? shouldn't this be equal since we pass proto as the this arg? I guess the only apparent reason to me would be saving a couple of bytes. But if using Object.prototype.hasOwnProperty is more correct, we should go for it!

Well there is https://eslint.org/docs/latest/rules/no-prototype-builtins which is "fixed" by this, not sure how important this is though 🤔

Hmm, so there does seem to be a difference there 🤔 changing this changes the browser integration tests, target of the mechanism becomes Window or Node instead of EventTarget 🤔 not sure why, but I guess I'll leave this as it was then!

TIL, thanks! No objections to leaving it as-is :D

mydea · 2024-11-25T16:05:57Z

packages/google-cloud-serverless/src/utils.ts

@@ -17,14 +17,14 @@ export function domainify<A extends unknown[], R>(fn: (...args: A) => R): (...ar
 * @returns wrapped function
 */
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-export function proxyFunction<A extends any[], R, F extends (...args: A) => R>(
+export function proxyFunction<F extends (...args: any[]) => unknown>(


sadly this somehow still has to be any, not quite sure why but didn't feel like investing more time into this...

Lms24

Generally LGTM! I like the type refactors. Had a concern with the GCP package type changes there. Can we extract these to their own PR (potentially when working on v9) and merge the browser/core changes in first?

Lms24 · 2024-11-25T16:26:17Z

packages/google-cloud-serverless/src/gcpfunction/general.ts

h: unfortunately, these types are public API because wrapHttpFunction, wrapCloudEventFunction and wrapEventFunction are exported and reference the types in their signature. Maybe we do this change in v9 instead?

Lms24 · 2024-11-25T16:27:20Z

packages/browser/test/helpers.test.ts

@@ -56,16 +64,6 @@ describe('internal wrap()', () => {
    expect(wrap(wrapped)).toBe(wrapped);
  });

-  it('calls "before" function when invoking wrapped function', () => {


l: why did we remove this test? (I guess it's redundant but just wanted to double-check)

I removed the before option from the function, it was not used anymore :)

Lms24 · 2024-11-25T16:30:44Z

packages/browser/src/integrations/browserapierrors.ts


-  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, no-prototype-builtins
-  if (!proto || !proto.hasOwnProperty || !proto.hasOwnProperty('addEventListener')) {
+  if (!proto || Object.prototype.hasOwnProperty.call(proto, 'addEventListener')) {


what's the difference between the two? shouldn't this be equal since we pass proto as the this arg? I guess the only apparent reason to me would be saving a couple of bytes. But if using Object.prototype.hasOwnProperty is more correct, we should go for it!

packages/browser/src/helpers.ts

mydea · 2024-11-26T08:17:57Z

I extracted the gcp stuff out into #14476 👍

mydea self-assigned this Nov 25, 2024

mydea commented Nov 25, 2024

View reviewed changes

mydea marked this pull request as ready for review November 25, 2024 16:06

mydea requested review from Lms24 and s1gr1d November 25, 2024 16:06

Lms24 reviewed Nov 25, 2024

View reviewed changes

mydea force-pushed the fn/wrap-types branch from e6e5c04 to 4d218ad Compare November 26, 2024 08:16

mydea mentioned this pull request Nov 26, 2024

ref(google-cloud-serverless/v9): Update event functions to avoid any #14476

Draft

mydea added 2 commits November 28, 2024 09:16

ref(types): Avoid some any type casting around wrap code

b9bf16c

some fixes

ab39fe8

mydea force-pushed the fn/wrap-types branch from 28b3c20 to ab39fe8 Compare November 28, 2024 08:16

mydea requested a review from Lms24 November 28, 2024 09:56

Lms24 approved these changes Nov 28, 2024

View reviewed changes

mydea merged commit be73e38 into develop Nov 28, 2024
153 checks passed

mydea deleted the fn/wrap-types branch November 28, 2024 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(types): Avoid some `any` type casting around `wrap` code #14463

ref(types): Avoid some `any` type casting around `wrap` code #14463

mydea commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

mydea Nov 25, 2024

mydea Nov 25, 2024

mydea Nov 25, 2024

Lms24 Nov 25, 2024 •

edited

Loading

mydea Nov 26, 2024

mydea Nov 26, 2024

Lms24 Nov 28, 2024

mydea Nov 25, 2024

Lms24 left a comment

Lms24 Nov 25, 2024 •

edited

Loading

Lms24 Nov 25, 2024

mydea Nov 26, 2024

Lms24 Nov 25, 2024 •

edited

Loading

mydea commented Nov 26, 2024

ref(types): Avoid some any type casting around wrap code #14463

ref(types): Avoid some any type casting around wrap code #14463

Conversation

mydea commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 • edited Loading

size-limit report 📦

codecov bot commented Nov 25, 2024 • edited Loading

❌ 1 Tests Failed:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 Nov 25, 2024 • edited Loading

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

Lms24 left a comment

Choose a reason for hiding this comment

Lms24 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

mydea commented Nov 26, 2024

ref(types): Avoid some `any` type casting around `wrap` code #14463

ref(types): Avoid some `any` type casting around `wrap` code #14463

github-actions bot commented Nov 25, 2024 •

edited

Loading

codecov bot commented Nov 25, 2024 •

edited

Loading

Lms24 Nov 25, 2024 •

edited

Loading

Lms24 Nov 25, 2024 •

edited

Loading

Lms24 Nov 25, 2024 •

edited

Loading