Problematic `PolymorphicRequest` Type?

[lforst]

Looking at https://github.com/getsentry/sentry-javascript/pull/5744, the PolymorphicRequest type seems wrong or rather dangerous to me. IMO this type needs to be a union (|) instead of an intersection (&).

https://github.com/getsentry/sentry-javascript/blob/9862a32899a41a3af9a5cd87c0d4a09a1d689e25/packages/types/src/polymorphics.ts#L16-L21

Why?: With an intersection, we're creating a new type that none of the frameworks support. This might lead to us making wrong assumptions about what a request might look like. For example, let's say a ExpressRequest has a field someField: string, but none of the others have someField, TS will still allow us to access someField, even though none of the others have it. (Not the best example since all of the fields we define are optional - I know... but still valid)

What would I do?: Replace this type with a union and remove the optional fields from the framework-specific Request types (unless they're actually optional). That way we can apply proper type-safe type narrowing in all the places where we accept a PolymorphicRequest.

We probably don't have any actual errors because of this, but in its current state, this is very error-prone.

I am opening this in a separate issue so we can discuss this regardless of open PRs.

[lobsterkatie]

I think this is a fair point. It's true that as written it's pretty loosey-goosey - there's no reason you couldn't pass a weird express/koa/nextjs hybrid, for example.

That said, IDK either how often that would ever really happen or whether or not it would cause any problems. My gut says "pretty much never" to the first one, and our current use of this type implies "no" to the second, since all we do is pull data from wherever it happens to be. (Which explains why nothing has gone wrong thus far.)

But you're absolutely right that we don't know how we'll handle the type in the future, and the intersections in the individual types take care of the combinations between them, so I think it's fine to use | rather than &. At the time I was replacing a single type which was called ExpressRequest but which had properties from all of the frameworks, and which similarly was just a conglomeration of optional attributes. And what you're pointing out is that I fixed the first problem but didn't do anything about the second, which again, I think is a fair point.

However: Changing it is unfortunately not as simple as switching all the &s to |s. When you do that, this is what you get:

So making the change will require changing the implementations in requestData.ts as well. Since we've already got a ticket tracking that, I'm going to add this to it, and we can come back to it when we tackle that. Sound reasonable?

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀