[$250] [Cleanup] Utility URL type for getURLWithBackParam, etc...

[roryabraham]

Coming from #31543 (comment)...

Problem

We've got some types that look like this:

function getUrlWithBackToParam<TUrl extends string>(url: TUrl, backTo?: string): `${TUrl}` | `${TUrl}?backTo=${string}` | `${TUrl}&backTo=${string}` {
    ...
}

and while that works it's a pretty verbose way to express a pretty common format for something.

Solution

This is a rich representation of what this string could / should look like, instead of just being string, but let's clean this up to make it more extensible/reusable. We should make a utility type for URLs that takes:

• A base URL
• an object type representing a series of optional or required query params (which of course can be in any order)

Then the resultant type would be any string matching the pattern of a valid URL with the correct base and query params.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01ab405b1ad68ca814
• Upwork Job ID: 1729557018701299712
• Last Price Increase: 2023-11-28

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01ab405b1ad68ca814

[melvin-bot]

Triggered auto assignment to @MitchExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @jjcoffee (External)

[melvin-bot]

Upwork job price has been updated to $250

[roryabraham]

Removing Help Wanted as @blazejkustra is going to work on this

[blazejkustra]

Hey! I’m Blazej from Software Mansion, an expert agency, and I’d like to work on this issue!

[hamzasaleem2]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Making URLs in code is complex and hard to read with the current getUrlWithBackToParam function.

What is the root cause of that problem?

The existing function is too detailed and prone to mistakes.

What changes do you think we should make in order to solve the problem?

Introduce a new tool, UrlWithParams, and a simpler function, getUrlWithParams, for easier and clearer URL construction.

Will just need to add a stringify function with object entries to be mapped together.

only this piece of code will be replaced with two functions.

function getUrlWithBackToParam(url: string, backTo?: string): string {
    const backToParam = backTo ? `${url.includes("?") ? "&" : "?"}backTo=${encodeURIComponent(backTo)}` : '';
    return url + backToParam;
}

[melvin-bot]

📣 @hamzasaleem2! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[hamzasaleem2]

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/freelancers/hamzasaleem

[melvin-bot]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[blazejkustra]

@hamzasaleem2 I'll work on this, as Rory mentioned here. Just need a little more time as I have some other priorities atm.

[roryabraham]

Not accepting other proposals for this as this issue was deferred feedback from a PR review

[melvin-bot]

@jjcoffee, @MitchExpensify, @blazejkustra, @roryabraham Eep! 4 days overdue now. Issues have feelings too...

[blazejkustra]

Not overdue, taking a look today 😄

edit: I have a PoC, will share it tomorrow.

[blazejkustra]

@roryabraham I took a different approach than the one described in the issue. After examining getUrlWithBackToParam usage I came to conclusion that a more reusable function is necessary that handles not only backTo param but it's generic enough to parse others params as well. Instead of focusing on a URL utility type:

type URL<TUrl extends string, TQuery extends Record<string, string>> = 
    | `${TUrl}?${TQuery['key1']}=${string}&${TQuery['key2']}=${string}...` 
    | `${TUrl}?${TQuery['key2']}=${string}&${TQuery['key1']}=${string}...`
    | ... ;

I think such granularity isn't needed when it comes to query params. It is far less readable compared to a simple ${TUrl}?${string}. As a result I implemented getUrlWithBackToParam, which you can check in this draft PR.

type QueryParams = Record<string, string | number | boolean | undefined>;

declare function getUrlWithQueryParams<TUrl extends string, TQuery extends QueryParams>(url: TUrl, query: TQuery): `${TUrl}?${string}`;

Examples:

getUrlWithQueryParams('r/123', {backTo: "value", arg: "value2"}) 
// results in type `r/123?${string}` | `r/123`

getUrlWithQueryParams(`settings/profile/personal-details/address/country`, {backTo, country})
// results in type `settings/profile/personal-details/address/country?${string}` | `settings/profile/personal-details/address/country`

cc'ing @BartoszGrajdek @fabioh8010, what do you think?

[fabioh8010]

@blazejkustra I think your solution is enough for our use cases, I don't see too much value in being super explicit about the query params in the string type, just ${TUrl}?${string} seems to be sufficient.

[BartoszGrajdek]

@blazejkustra I think it makes sense. Although we have been quite strict in typing in some cases, this could be an exception. The Navigation is already complicated enough as it is, and adding more advanced and strict types than necessary will make it harder for us to refactor/work later on.

[roryabraham]

I personally think that it would be more valuable to be explicit about which URL params, specifically, are expected/allowed for a specific route, and what specifically their type should be.

@blazejkustra The first code snippet you showed looked best to me, though I wonder if there's a way to DRY it up / generalize it a bit rather than having to manually list out all possible arrangements when we have potentially N query params.

[blazejkustra]

I personally think that it would be more valuable to be explicit about which URL params

@roryabraham Maybe I'm missing something, what would be the benefit of such strictness? 🤔

it would be more valuable to be explicit about which URL params specifically, are expected/allowed for a specific route, and what specifically their type should be.

I think it's already handled quite well, each getRoute function defines query parameters for each route.

    EXAMPLE: 'example', // no query params for example route
    DETAILS: {
        route: 'details',
        getRoute: (login: string) => getUrlWithQueryParams('details', {login}),
    }, // details route requires a login query param
    PROFILE: {
        route: 'a/:accountID',
        getRoute: (accountID: string | number, backTo?: string) => getUrlWithQueryParams(`a/${accountID}`, {backTo}),
    }, // profile route has an optional backTo query param

The first code snippet you showed looked best to me, though I wonder if there's a way to DRY it up / generalize it a bit rather than having to manually list out all possible arrangements when we have potentially N query params.

Listing all the possible combinations of query parameters in a template-literal string won't be done manually by listing all possibilities. I just wanted to show what would be the resulting type. I didn't code it as it's not trivial 😅

[roryabraham]

what would be the benefit of such strictness?

My thinking is that you'd be unable to pass unexpected params or miss required params when navigating to a screen. But it sounds like I'm alone in thinking this would bring any additional value, so I'll trust you guys and close this out.