PhilanthropyDataCommons / service

A project for collecting and serving public information associated with grant applications
GNU Affero General Public License v3.0
8 stars 2 forks source link

Make offset and limit required, named parameters for bundle functions #1186

Closed slifty closed 1 month ago

slifty commented 1 month ago

This is a two-part issue.

  1. Our bundle loaders currently take a queryParameters object. Let's move away from that pattern and into having a function parameter signature such that we have required parameters as true parameters, with a potential options parameter if there are any optional parameters.

  2. Let's make offset and limit required parameters (no longer passed via queryParameters

Thus we'd move something like:

export const loadBaseFieldLocalizationsBundle = async (queryParameters: {
    offset?: number;
    limit?: number;
    baseFieldId?: number;
}): Promise<Bundle<BaseFieldLocalization>> => ...

to

export const loadBaseFieldLocalizationsBundle = async (
        offset: number;
    limit: number;
    options: {
         baseFieldId?: number;
    }
}): Promise<Bundle<BaseFieldLocalization>> => ...
slifty commented 1 month ago

@jasonaowen @bickelj @hminsky2002 I wanted to ping you on the above to see if you have any opinions on the above proposed pattern.

My thinking behind having options vs simply having baseFieldId as an optional true parameter is that way we can stay consistent across operation signatures in the event that there is more than one optional parameter.

One downside to parameters instead of an options object with named parameters is that it's easier to accidentally make interpolation errors (e.g. pass limit as offset, or offset as limit)

jasonaowen commented 1 month ago

That looks good to me!

One downside to parameters instead of an options object with named parameters is that it's easier to accidentally make interpolation errors (e.g. pass limit as offset, or offset as limit)

That is a risk, although we should be able to mitigate it slightly by being consistent with the ordering.

Depending on how worried we are about this, we could make a newtype to encapsulate it, perhaps via something like newtype-ts (note I have not done an exhaustive search of alternatives). It gets a bit wonky, and I'm not sure the complexity is worth the benefit, but it would definitely defend against this and other just-a-number type errors!