ConfigurableModuleBuilder doesn't resolve extras async

[tonyd33]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

ConfigurableModuleBuilder does not resolve extras set on .setExtras in the async version. The extras passed to the transformationDefinition is always the default using the async version.

Minimum reproduction code

https://codesandbox.io/p/devbox/restless-snow-6y9q4y?workspaceId=ws_Wb56io5ZgKjPm8EKBAcX43

Steps to reproduce

See the codesandbox. The only relevant changed files from the starter project are in src/app.module.ts and src/config/

Expected behavior

look at the terminal output on the codesandbox: I'd expect to see an output like:

{ folder: 'forRoot' }
{ folder: 'forRootAsync' }

yet I get

{ folder: 'forRoot' }
{ folder: 'default', useFactory: [Function: useFactory] }

separately, you can see in the same terminal output that it seems like the the options aren't making it into the ConfigService correctly...? this issue isn't centered around that though

NestJS version

11.0.1

Packages versions

➜  workspace git:(main) npx nest info

 _   _             _      ___  _____  _____  _     _____
| \ | |           | |    |_  |/  ___|/  __ \| |   |_   _|
|  \| |  ___  ___ | |_     | |\ `--. | /  \/| |     | |
| . ` | / _ \/ __|| __|    | | `--. \| |    | |     | |
| |\  ||  __/\__ \| |_ /\__/ //\__/ /| \__/\| |_____| |_
\_| \_/ \___||___/ \__|\____/ \____/  \____/\_____/\___/


[System Information]
OS Version     : Linux6.1.43
NodeJS Version : v20.12.1
NPM Version    : 10.5.0 

[Nest CLI]
Nest CLI Version : 11.0.0 

[Nest Platform Information
platform-express version : 11.0.1
schematics version       : 11.0.0
testing version          : 11.0.1
common version           : 11.0.17
core version             : 11.0.1
cli version              : 11.0.0

pretty sure this is the same on the latest release as well

Node.js version

20.12.1

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

No response

[tonyd33]

oh i guess we can pass the extras within the toplevel forRootAsync object itself like:

    ConfigModule.forRootAsync({
      folder: 'forRootAsync',
      useFactory: () => {
        return { folder: 'forRootAsync' };
      },
    }),

this does produce the expected output in the transformationDefinition, but surely this is not the intended way to do this

[kamilmysliwiec]

Would you like to create a PR for this issue?

[tonyd33]

Would you like to create a PR for this issue?

i'd need some advice on the approach

[mag123c]

I Think, In registerAsync, the entire options object (including useFactory, inject, etc.) is passed to transformModuleDefinition, but it should only receive user-defined extras.

it('should preserve extras in registerAsync transformation', () => {
  let capturedExtras: any;

  const { ConfigurableModuleClass } = new ConfigurableModuleBuilder()
    .setExtras(
      { folder: 'default' },
      (definition, extras: { folder: string }) => {
        capturedExtras = extras;
        return {
          ...definition,
          customProperty: `folder: ${extras.folder}`,
        };
      },
    )
    .build();

  const asyncResult = ConfigurableModuleClass.registerAsync({
    useFactory: () => ({}),
    folder: 'forRootAsync',
  });

  expect(capturedExtras).to.deep.equal({ folder: 'forRootAsync' });
  expect(asyncResult).to.have.property(
    'customProperty',
    'folder: forRootAsync',
  );
});

The test fails because capturedExtras contains { folder: 'forRootAsync', useFactory: [Function] } instead of just { folder: 'forRootAsync' }.

Maybe would be to filter out async-specific keys before passing to the transformation function.

[tonyd33]

i dont think i understand the intended behavior. I would think that with this setup...

const asyncResult = ConfigurableModuleClass.registerAsync({
  useFactory: () => ({ folder: 'forRootAsync' })
})

...the capturedExtras in your example should be { folder: 'forRootAsync' }. in other words, I thought the return of useFactory in .forRootAsync should be what would have gone in .forRoot. this is what the documentation suggests: https://docs.nestjs.com/fundamentals/dynamic-modules#configurable-module-builder

[tonyd33]

are we saying that the intended behavior is that the extras should be set at the top level object of what's passed in .forRootAsync? That seems to be at odds with the documentation and what I'm used to for existing libraries

[tonyd33]

ah, actually this is not clear anymore now that i'm reading the extras documentation more closely. either way, i'd like to clarify the intended behavior before I work on a change

[mag123c]

@tonyd33
I think there's some confusion about the intended behavior.

1. Module Options (from useFactory): The actual configuration for your module
2. Extras: Additional metadata used to transform the module definition

ConfigurableModuleClass.registerAsync({
  useFactory: () => ({
    apiKey: 'secret',      // These are module options
    timeout: 5000
  }),
  folder: 'forRootAsync',  // This is an extra
  isGlobal: true          // This is also an extra
})

The extras (folder, isGlobal) should be passed to your transformation function, while useFactory return value goes to your module's injection token.

See the existing test: https://github.com/nestjs/nest/blob/master/packages/common/test/module-utils/configurable-module.builder.spec.ts#L97

const definition = ConfigurableModuleClass.forFeatureAsync({
    useFactory: () => {},           // Module options factory
    inject: ['a'],                  // Async-only option
    provideInjectionTokensFrom,     // Async-only option
    isGlobal: true,                 //  EXTRA
    extraProviders: ['test'],       //  EXTRA
});

expect(definition.global).to.equal(true);  // isGlobal extra transformed the module

This clearly shows that isGlobal and extraProviders are treated as extras, while useFactory/inject are async-specific options.

[tonyd33]

@mag123c i see, thanks for clarifying. it seems all that needs to be done is change the types so that useFactory isn't typed to return stuff set in extras, and then only pass the stuff set in extras into transformModuleDefinition then, correct?

[mag123c]

@tonyd33 planning to open a PR for this issue? If not, I’d be happy to take it on.