fix: Update ExtractedNodeRequestData to include valid query_params for tracesSampler

[kamilogorek]

Fixes #3714

Fwiw we should somehow unify ExtractedNodeRequestData and Request types, as they are almost identical.

It should™ be a non-breaking change, as the broader type includes string and it's never used directly by the user.

[rhcarvalho]

I think the safer change would be to fix the implementation in @sentry/nextjs:

sentry-javascript/packages/nextjs/src/utils/instrumentServer.ts

Line 323 in 345fda7

query_string: req.query,

It should not pass req.query (map-like) directly.

[github-actions]

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	21.39 KB (-0.01% 🔽)
@sentry/browser - Webpack	22.4 KB (0%)
@sentry/react - Webpack	22.44 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	28.89 KB (-0.01% 🔽)

[kamilogorek]

Not sure how feasible it is, so cc @lobsterkatie @AbhiPrasad @iker-barriocanal

[rhcarvalho]

The QueryParams type was introduced rather recently in https://github.com/getsentry/sentry-javascript/pull/3580/files#diff-64477348658a923e34c3ce4e902c93d1de079bafab9322ee194f2de5809d7eb9R22

And it has the downside of forcing users to check the type in runtime before using it, because you can get 1 of 3 types... harder to use than our original state.

[lobsterkatie]

The QueryParams type was introduced rather recently in #3580 (files)

@rhcarvalho - The type change was made to match the unified SDK spec.

If we can only pass it as a string, we'll have to artificially construct a string of the form key1=value1&key2=value2, since in next.js the req.query object includes not only anything from the original URL's actual query string but also any parameters included as part of the URL itself. (So, in other words, if the URL is /dogs/maisey/tricks?trickName=kangaroo and the route is dogs/[dogName]/tricks, then req.query is { dogName: 'maisey', trickName: 'kangaroo' }. Obviously this is doable, but given that the spec allows an object, it feels like work we shouldn't have to do.

Thoughts?

And it has the downside of forcing users to check the type in runtime before using it, because you can get 1 of 3 types... harder to use than our original state.

For any given framework, it should always be the same type, so no type-checking ought to be necessary. Can you think of an instance where that wouldn't be true?

[iker-barriocanal]

An object is easier to deal with than a query string, and if the spec allows it...

[rhcarvalho]

An object is easier to deal with than a query string, and if the spec allows it...

Yes, an object might be easy to work with, but a union type not as much. Because it is not always an object, user code needs to handle all possible types.

[rhcarvalho]

The type change was made to match the unified SDK spec.

The spec is what Relay accepts, doesn't necessarily mean that SDKs should send data in all possible types.

For any given framework, it should always be the same type, so no type-checking ought to be necessary. Can you think of an instance where that wouldn't be true?

Hmm, I don't think that's the case in TS. See https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABMAFAQwE4HMBciDOUGMYWiAPgUSVgNoC6DFiA3orQNYCmAnnocVL1+1UgG5EAXwCUrAFCJFiCAnxwANlwB06uFnTYtJACZcAHgHlUAcjTXp0sXMly5oSLASJ9mXFUFYsiwKSipgapo6egZYRmCmljZ2Dk4ubijWAFZoYFwwGHD2Tvq2AF55UFxFrkA

function f(arg: string | string[][] | { [key: string]: string; }) {
    console.log(arg.indexOf('a'));
}

function g(arg: string) {
    console.log(arg.indexOf('a'));
}

f('janeiro');
g('azeite');

This expression is not callable.
  Not all constituents of type 'string | ((searchString: string, position?: number | undefined) => number) | ((searchElement: string[], fromIndex?: number | undefined) => number)' are callable.

So even though you expect that under certain circumstances (e.g. in @sentry/nextjs) the type is always a certain type that is part of the union, you'd still need to write something to convince the compiler, and then cross-fingers that your expectation/assumption was right and you won't introduce a runtime error.

[rhcarvalho]

My comments just reflect my views on usability and compatibility, but I don't have a strong stance on this, please feel free to disagree and move on with any decision.

[lobsterkatie]

I say we go with it and revisit only if someone complains.