Skip to content

Conversation

@keawade
Copy link

@keawade keawade commented Nov 4, 2024

Howdy @benhason1, I've been happily using this library in my work for a while now. Thanks!

In my work I've found myself needing to configure Axios interceptors up front but not finding a good way to do that between NestJS's constraints and the current implementation of this module. This PR aims to provide a way for users to configure Axios interceptors via the Module registration.

To do this, this PR introduces a breaking change to how the module is configured. The module options object is no longer the Axios configuration object but rather a module options object of

type HttpModuleOptions = {
  axiosConfig: AxiosRequestConfig;
  axiosRetryConfig: IAxiosRetryConfig;
  interceptors: {
    // Full details added to documentation in the readme
    request: { ... };
    response: { ... };
  };
}

I'm not a huge fan of breaking changes but I found myself preferring this approach to overloading the existing configuration object. Since I was splitting something out from the shared Axios and Axios Retry config object anyways I figured it might be worth additionally explicitly disentangling those two configuration objects as well which should have the additional benefit of lowering the risk of name collisions between the two libraries.


I refactored the module to use NestJS's ConfigurableModuleBuilder make it much simpler for me to add the new module configuration options.

This should also mean you won't have to maintain your own implementations of .register and .registerAsync any more as that is now handed off to NestJS's builder.

I kept the bulk of these changes in the first commit to help make it easier to review.


To make sure I didn't break anything unexpected I added tests of the module construction. I used vitest as it is one of the most Just Works TM solutions I'm aware of.


It appeared the documented "better axios stack trace" is no longer implemented by this library directly so I removed references to it in the documentation. As far as I can tell the related issues in the Axios repo have been resolved and improvements to the stack trace have been merged.


Let me know if you have any questions or suggestions for how to improve this PR! Thanks again!

@keawade keawade force-pushed the up-front-interceptor-config branch from 8c5ac5a to 950771f Compare November 4, 2024 21:40
@keawade
Copy link
Author

keawade commented Nov 4, 2024

Just added a workflow to run tests against the three node versions that are receiving updates currently. You can see an example of what it will look like on this test PR here: keawade#1

Example run: https://github.com/keawade/nestjs-http-promise/actions/runs/11673157112?pr=1

Trying to fix issue installing in new NestJS project created with `nest new`
@keawade
Copy link
Author

keawade commented Nov 8, 2024

Hey @benhason1 👋. Bumping this in the hopes it could get some discussion or maybe even a merge. 😄

@benhason1 benhason1 added the feature feature to add label Nov 9, 2024
@benhason1 benhason1 self-assigned this Nov 9, 2024
@benhason1
Copy link
Owner

Hi @keawade thanks for the PR, I will look at it in the coming days

@benhason1
Copy link
Owner

benhason1 commented Feb 22, 2025

Thanks for the PR @keawade, Srry for the delay.
First of all, I agree about the separation of keys. that's indeed looks better than the previous approach.
The Readme changes and the tests added also looks very good.

Just to things to address so we can merge it and publish a new version.

  • I added some comments for reviews on the PR
  • There are some merge conflicts because of an earlier merge to main (minor changes of upgrading nestjs to version 11)

After you will fix those things I will be more than happy to merge it into main :)

createHttpOptions(): Promise<HttpModuleOptions> | HttpModuleOptions;
}
interceptors?: {
request?: {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why you're creating the this type manually and not using AxiosInterceptorManager.use type?

export class HttpModule extends ConfigurableModuleClass {
// Redefining the register static method to allow `undefined` options to be
// accepted by the type system
public static register(options?: HttpModuleOptions): DynamicModule {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you call register with undefined instead of just not calling register()?

const moduleRef = await Test.createTestingModule({
imports: [
HttpModule.registerAsync({
useFactory: () => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pass an async method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature feature to add

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants