Add optional errorMessage argument to defaultOnLimit

[ewen-lbh]

This is useful if e.g. your API is localized in a different language than English.

Currently, to change the error message, you either have to try / catch the defaultOnLimit and re-throw yourself, or just not call defaultOnLimit. But as a library user, before looking at the source code, I didn't know that defaultOnLimit just throws an error and does nothing else.

[ravangen]

Thank you for the proposal, but I'm not sure it is necessary.

Currently defaultOnLimit builds an error message with contextual data and throws an GraphQLError. If errorMessage was an additional, optional argument, its value would be static and miss the contextual data of when you can try again.

Alternatively, define a custom onLimit function that has the static error message you desire? Similar to example

// Specify how a rate limited field should behave when a limit has been exceeded
const onLimit = (resource, directiveArgs, source, args, context, info) => {
  throw new GraphQLError("My localized message");
};

const { rateLimitDirectiveTypeDefs, rateLimitDirectiveTransformer } = rateLimitDirective({
  onLimit,
});

In order to pass a custom errorMessage to defaultOnLimit, you would already need to specify onLimit key for rateLimitDirective(). Given the simplicity of defaultOnLimit, I am hesitant to increase the complexity of the defaultOnLimit() to accept a custom static error string instead of forcing you to provide a customized limit function.

[ewen-lbh]

If errorMessage was an additional, optional argument, its value would be static and miss the contextual data of when you can try again.

Not if called while overriding onLimit, which I guess is the main use case for calling defaultOnLimit (if not the only?)

Given the simplicity of defaultOnLimit, I am hesitant to increase the complexity of the defaultOnLimit() to accept a custom static error string instead of forcing you to provide a customized limit function.

That's the thing: the users of the library can't know that defaultOnLimit only throws an error message, and does not do other default behavior (like mutating some internal state of the library that we don't know about, that's what I was concerned about when writing my code) that we may not want to override.

An other option would be to document that defaultOnLimit does nothing else than throw an error (and therefore is safe to not call when overriding onLimit); but that kind of forces you to not modify that function in the future without a major release.

Just to be clear, my current code does redefine onLimit, but customizing the error message without making any assumptions about the implementation of defaultOnLimit requires catching then re-throwing:

...
    async onLimit(response, dargs, src, args, ctx, info) {
      // ... unrelated Prometheus stuff ...
      try {
        defaultOnLimit(response, dargs, src, args, ctx, info);
      } catch (error) {
        const error_ =
          error instanceof GraphQLError
            ? new GraphQLError(
                `Trop de requêtes. Réésaye dans ${formatDuration(intervalToDuration({ start: 0, end: response.msBeforeNext }), { locale: fr })}`,
              )
            : error;
        throw error_;
      }
...

But to be honnest this is a minor issue, so if you don't want to change anything I'll just leave my code like, it's not that big of a deal. It's just something I thought of.

[ravangen]

Thank you for providing an example of what you are doing. At this point, I do not believe accepting errorMessage as part of defaultOnLimit to be a meaningful improvement relative to maintenance overhead.

The intention with onLimit hook was to provide an extension point so that the entire behaviour to be swapped out. As a result, no assumptions are made about what the implementation does. For example, it could return a custom error or custom object. It doesn't need to be GraphQLError.

That's the thing: the users of the library can't know that defaultOnLimit only throws an error message, and does not do other default behavior (like mutating some internal state of the library that we don't know about, that's what I was concerned about when writing my code) that we may not want to override.

Fair. As a library author, if something extra was always necessary, I would seek to put that before/after the hook. This way I do not depend on library consumers to include such behaviour in their custom logic. For example, saving state...

https://github.com/ravangen/graphql-rate-limit/blob/9e4de79d0610ebff8cb10f803e00f716b869c81b/src/index.ts#L315-L316

An other option would be to document that defaultOnLimit does nothing else than throw an error (and therefore is safe to not call when overriding onLimit); but that kind of forces you to not modify that function in the future without a major release.

Where and how would you like to see the description improved? I already should not change defaultOnLimit given it is documented to raise GraphQLError.

https://github.com/ravangen/graphql-rate-limit/blob/9e4de79d0610ebff8cb10f803e00f716b869c81b/README.md?plain=1#L233-L237

https://github.com/ravangen/graphql-rate-limit/blob/9e4de79d0610ebff8cb10f803e00f716b869c81b/README.md?plain=1#L301-L305

https://github.com/ravangen/graphql-rate-limit/blob/9e4de79d0610ebff8cb10f803e00f716b869c81b/src/index.ts#L118-L121

https://github.com/ravangen/graphql-rate-limit/blob/9e4de79d0610ebff8cb10f803e00f716b869c81b/src/index.ts#L207-L216

[ewen-lbh]

Welp yeah this is embarrasing lol it's already pretty explicit maybe mention on the jsdoc of defaultOnLimit that it can be safely skipped when overriding onLimit, but at this point it's kinda my fault for not paying enough attention '^^

Sorry!

[ravangen]

No problem, thank you for reaching out none the less 😄