Skip to content

fix(replay): Fixes type error if same param is in url #68851

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

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

c298lee
Copy link
Contributor

@c298lee c298lee commented Apr 13, 2024

There are edge cases where getParamValue() will have a return type of string[] due to location.query[key] which caused the type error. This edge case happens when the same key is in the URL more than once, which could happen if someone pastes the url twice.
We are expecting a string from this function, so if location.query[key] is an array, we will just return the first value in the array.

Fixes JAVASCRIPT-2SFK

@c298lee c298lee requested a review from ryan953 April 13, 2024 06:30
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 13, 2024
@c298lee c298lee changed the title fix(replay): Fixes type error if getParamValue returns type string[] fix(replay): Fixes type error if same param is in url Apr 13, 2024
Copy link

codecov bot commented Apr 13, 2024

Bundle Report

Changes will decrease total bundle size by 33.53kB ⬇️

Bundle name Size Change
sentry-webpack-bundle-array-push 26.27MB 33.53kB ⬇️

@c298lee c298lee requested a review from a team April 15, 2024 14:24
Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is valid behavior, we shouldn't force this in the hook, but it should be handled in the hook consumers

@c298lee
Copy link
Contributor Author

c298lee commented Apr 15, 2024

This is valid behavior, we shouldn't force this in the hook, but it should be handled in the hook consumers

We currently have the return type of the hook set to string instead of string | string[] so currently the hook consumers assume that there will only be unique param keys. I decided to put it in the hook since this would require more changes if done in the hook consumer, but I can change it to the consumer instead

@@ -20,7 +20,9 @@ function useUrlParams(defaultKey?: string, defaultValue?: string) {
const getParamValue = useCallback(
(key: string) => {
const location = browserHistory.getCurrentLocation();
return location.query[key] ?? defaultValue;
return Array.isArray(location.query[key])
? location.query[key]?.[0] ?? defaultValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
? location.query[key]?.[0] ?? defaultValue
? location.query[key]?.at(0) ?? defaultValue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of noUncheckedIndexedAccess

@billyvg
Copy link
Member

billyvg commented Apr 15, 2024

This is valid behavior, we shouldn't force this in the hook, but it should be handled in the hook consumers

We currently have the return type of the hook set to string instead of string | string[] so currently the hook consumers assume that there will only be unique param keys. I decided to put it in the hook since this would require more changes if done in the hook consumer, but I can change it to the consumer instead

OK, let's keep it as-is in that case since they are already expecting a singular value (the function name reinforces this). If they need an array, they can later add on a different getter.

Let's add a comment here about why we're doing this and why first value over last (even if it's we are arbitrarily picking one, it's plenty helpful), and also a test.

Comment on lines +24 to +26
return Array.isArray(location.query[key])
? location.query[key]?.at(0) ?? defaultValue
: location.query[key] ?? defaultValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk what's easier to read

      return (
        Array.isArray(location.query[key])
          ? location.query[key]?.at(0)
          : location.query[key]
        ) ?? defaultValue;

@c298lee c298lee merged commit 978068f into master Apr 15, 2024
42 checks passed
@c298lee c298lee deleted the cl/urlparams-typeerror branch April 15, 2024 19:18
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants