feat: Make getQueryParams more resilient
#1778
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
rafaeelaudibert
force-pushed
the
make-getQueryParams-more-resilient
branch
from
3caa128 to
7848a19
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Enhanced URL query parameter parsing in request-utils.ts to handle malformed URLs and improve browser compatibility.
- Added modern implementation using
URLandURLSearchParamsAPIs for better query parameter handling - Created fallback implementation for older browsers (IE11, op_mini) that trims leading question marks
- Split
getQueryParamsfunction into two parts for cleaner separation of modern and legacy code - Added handling for malformed URLs with multiple leading question marks
- Improved error resilience for edge cases in URL parsing
1 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
src/utils/request-utils.ts
Outdated
| export const getQueryParam = function (url: string, param: string): string { | ||
| // Check if URL and URLSearchParams are available in the global scope | ||
| // Fall back to manual parsing if URL/URLSearchParams aren't supported | ||
| if (typeof URL === 'undefined' || typeof URLSearchParams === 'undefined') { | ||
| return getQueryParamFallback(url, param) | ||
| } | ||
|
|
||
| // Disabling compat/compat here because we've checked its existence above | ||
| const urlObj = new URL(url) // eslint-disable-line compat/compat | ||
|
|
||
| // Remove leading ? from search string if present | ||
| // This is not really necessary if there's a single ? in the search string | ||
| // but it's helpful if someone has some malformed URL with multiple ?s | ||
| const search = urlObj.search.replace(/^\?*/g, '') | ||
| const searchParams = new URLSearchParams(search) // eslint-disable-line compat/compat | ||
|
|
||
| return searchParams.get(param) ?? '' | ||
| } |
There was a problem hiding this comment.
Open to feedback on whether this is needed. I think using the builtins are better than rolling our own code like we have on the fallback, but happy to be told otherwise and just stick to the previous thing we had going
There was a problem hiding this comment.
I'd rather only have 1 version. Bundle size is important. Sorry man =/ I'd love to kill off the old version and only have one that uses built-in methods, but if we need to support older browsers then we can't do that.
There was a problem hiding this comment.
Not a problem, will revert
|
Size Change: +275 B (+0.01%) Total Size: 3.56 MB
ℹ️ View Unchanged
|
rafaeelaudibert
force-pushed
the
make-getQueryParams-more-resilient
branch
2 times, most recently
from
47fe115 to
8c0bb38
Compare
src/utils/request-utils.ts
Outdated
| // Check if URL and URLSearchParams are available in the global scope | ||
| // Fall back to manual parsing if URL/URLSearchParams aren't supported | ||
| if (typeof URL === 'undefined' || typeof URLSearchParams === 'undefined') { | ||
| return getQueryParamFallback(url, param) |
There was a problem hiding this comment.
Is there any downside to just always calling that function? It's nicer to use the built-in parsing, and if we could guarantee that it was available, we'd just use that, but given we have to have a fallback anyway, why add extra bytes to the bundle with two implementations?
There was a problem hiding this comment.
Just because I'd prefer to use the built-in if it exists, but I'm fine with keeping the old-but-proven-to-work function instead, and just fix it (which I did anyway)
There was a problem hiding this comment.
Which is basically what I commented here: #1778 (comment)
There was a problem hiding this comment.
Would you prefer we kept that old code instead and never add the new one?
An SDK user has a slightly broken URL where they have two leading question marks, which is causing our `queryParam` parsing to break. Right now, because we split on `?` and get the second split, we're simply getting an empty string. Even though this is liekly user error, we're fixing this by trimming leading question marks to avoid this from happening again See https://posthoghelp.zendesk.com/agent/tickets/25469
rafaeelaudibert
force-pushed
the
make-getQueryParams-more-resilient
branch
from
8c0bb38 to
3e9745e
Compare
|
@robbie-c Ready for a re-review, I've removed the new code and simply adapted the old one |
An SDK user has a slightly broken URL where they have two leading question marks, which is causing our
queryParamparsing to break. Right now, because we split on?and get the second split, we're simply getting an empty string.We're fixing this in two ways:
See https://posthoghelp.zendesk.com/agent/tickets/25469