mollie-api-node icon indicating copy to clipboard operation
mollie-api-node copied to clipboard

Enhance Mollie Connect API Client Configuration for OAuth Flow

Open iFlyinq opened this issue 1 year ago • 10 comments

For the Mollie Connect API, we need to use the OAuth flow, which means we will provide an accessToken instead of an apiKey. When creating a client with an apiKey, everything is set up and ready to go, with no need to define profiles or test mode. However, when using an accessToken, it would be convenient to optionally specify a profileId and testmode since these are required for most API calls. This way, we can avoid injecting the profileId and testmode into the API call parameters at every implementation point.

Here's the updated TypeScript type definition to reflect this:

type Options = Xor<{
    /**
     * The Mollie API key, starting with `'test_'` or `'live_'`.
     */
    apiKey: string;
}, {
    /**
     * OAuth access token, starting with `'access_'`.
     */
    accessToken: string;
    /**
     * The profile ID associated with the access token.
     */
    profileId?: string;
    /**
     * Indicates whether the API should operate in test mode.
     */
    testmode?: boolean;
}> & {
    /**
     * One or an array of version strings of the software you are using, such as `'RockenbergCommerce/3.1.12'`.
     */
    versionStrings?: MaybeArray<string>;
    /**
     * The headers set in the requests sent to the Mollie API. `Authorization`, `User-Agent`, `Accept`,
     * `Accept-Encoding`, and `Content-Type` are set by this library directly. Setting them here has no effect.
     */
    headers?: Record<string, string>;
    /**
     * The URL of the root of the Mollie API. Default: `'https://api.mollie.com:443/v2/'`.
     */
    apiEndpoint?: string;
};

With this update, when using an accessToken, you can optionally provide a profileId and testmode to streamline the API calls without needing to inject these parameters manually each time. This approach enhances code readability and maintainability by centralizing the configuration of these essential parameters.

iFlyinq avatar Mar 28 '25 20:03 iFlyinq

Hi @iFlyinq,

i understand your motivation, but I don't believe this should be implemented.

This package aims to mimic the interface of mollies REST API as closely as possible. So since these are params for some (but not all) calls, they should be explicitly sent in those calls.

For this reason, but more importantly to avoid breaking changes, we definitely can't just remove these props from the individual calls and replace them with global ones.

So if at all, this could be an add on, maybe globalOverrides.

Even with this approach I would advise against it, because we'll get logic issues (what if both are set, which has precedence?) and more importantly transparency issues. It might become unclear where a param for a call comes from or that it was even set in the first place (it might be unintentional).

But I'm curious to hear other opinions / arguments?

janpaepke avatar Mar 31 '25 08:03 janpaepke

Totally get the motivation, @janpaepke. I had the same thought when making the issue but decided to give it a shot anyway. The frustrating part is that there's no way to override this behavior within the library—it’s completely closed. A simple global override could be a viable solution indeed.

iFlyinq avatar Mar 31 '25 08:03 iFlyinq

This package aims to mimic the interface of mollies REST API as closely as possible.

Yes, but we also look to provide some Quality of Life improvements here and there :)

Maybe there is a way where we can have the cake and eat it, too? I lack understanding of The Node Way™ to come up with a recommendation, though.

fjbender avatar Mar 31 '25 08:03 fjbender

Here's my 2c, curious what you think:

Context:
In OAuth mode (accessToken), you end up needing to send profileId and testmode on a lot of requests. In contrast, with API key auth, those are basically implied—so no need to specify them repeatedly.

Feels like:
It’d be really nice if we could instantiate the client with default values for these params. From a DX standpoint, this would save a lot of boilerplate for OAuth users.

That said, a few considerations:

  • Which params get defaults?
    If we go this route, we should probably scope it tightly. Start with profileId and testmode. Slippery slope if we try to support too many.

  • API surface vs API spec:
    This does (slightly) move us away from a 1:1 mirror of Mollie’s API. Devs might not realize a param is being set behind the scenes.
    We might be able to model this in TS—like “this param is required unless provided as a default”—but that could get hairy pretty quickly.

  • Implementation overhead:
    Can’t blanket-apply defaults everywhere—some endpoints don’t take these params. So we’d need to manually inject defaults where appropriate.
    That’s both upfront work (go through all endpoints) and ongoing effort (remember to apply defaults in new ones).

Overall take:
Still feels like a net positive. We’d just need to be disciplined about scope and maintenance. If we can push some of the smart typing later, maybe worth starting simple and iterating.

edorivai avatar Apr 01 '25 07:04 edorivai

I can't comment on the added complexity and implementation specifics, but in my opinion a good API client design keeps DX in mind. So if there's a way to get rid of lots of boilerplate for users with manageable increase in maintenance complexity, we should consider doing it.

hreinberger avatar Apr 01 '25 07:04 hreinberger

Allright, you convinced me.

There are three ways I can think of to implement this:

1. Overrides

The globally defined values will always override any values in the individual calls.

createMollieClient(options: {
    accessToken: string;
    overrides: {
      profileId?: string;
      testmode?: boolean;
    }
})

2. Defaults

The globally defined values will only be used, if not overridden within the individual calls.

createMollieClient(options: {
    accessToken: string;
    defaults: {
      profileId?: string;
      testmode?: boolean;
    }
})

3. Mutually Exclusive

You can set either the global value OR within the individual calls. We can craft some fancy types, which would make it so that in all its methods the you cannot set the respective option as a parameter. So if you provide 'profileId' globally, ts would not allow it on the individual calls anymore.

createMollieClient(options: {
    accessToken: string;
    profileId?: string;
    testmode?: boolean;
})

My strong recommendation would be Option 2. All of these are technically non-breaking, since nothing would happen, if no global values are provided. When defining global values, however, Option 2 is the only one that will keep current behaviour for existing calls with explicit values for these fields.

What are your thoughts?

janpaepke avatar Apr 02 '25 07:04 janpaepke

I agree that option 2 is strongly preferable.

fjbender avatar Apr 02 '25 09:04 fjbender

Yep, option 2 - 💯%. With the slight tweak of defaults?: {...} 😉 - but that's probably what you meant anyway.

edorivai avatar Apr 02 '25 10:04 edorivai

And just for the record, this would be the coolest TS experience IMO:

  • Instantiating a client requires an auth method + corresponding secret (e.g. api key, oauth access token, organization token)
  • Based on the auth method, profileId and testmode either become required (oauth, org token) or disallowed (api key). I assume passing those params with api key is not allowed, right?
    • If the fields are technically allowed, but practically ignored, we can mark the params @deprecated in jsdoc -- that would give users a heads-up in their IDE when setting them. Would smoothe over the release, cause disallowing would be a breaking change.
    • However, I wonder... an API Key is tied to a specific profile, so setting the profileId param to anything other than the profile the key is tied to would always result in unauthorized right? So from an SDK perspective, I think disallowing (or deprecating + ignoring) would actually be better.
  • You can define defaults when instantiating a client
  • If defaults were set, that makes the fields optional on the endpoints.

Again - just writing this out so it's on the table. I'm fully aware that this would require some very non-trivial TS magic which could be a lot of work, but it would be the best DX in my opinion 🙃.

edorivai avatar Apr 02 '25 10:04 edorivai

So we agree that this is a useful enhancement.

Unfortunately due to current time constraints there is no clear schedule for implementation at this point.

If this is urgent or critical to any reader, feel free to submit a PR to potentially speed up this process.

janpaepke avatar Apr 07 '25 08:04 janpaepke