Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Readonly ConfigType #1387

Closed
1 task done
yura2100 opened this issue Jul 19, 2023 · 2 comments
Closed
1 task done

Readonly ConfigType #1387

yura2100 opened this issue Jul 19, 2023 · 2 comments

Comments

@yura2100
Copy link

yura2100 commented Jul 19, 2023

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

Using Configuration namespaces is pretty powerful as it enhances type safety and reduces amount of boilerplate code. However, configuration object is a mutable singleton shared across the whole application. There are no limitations for mutating configuration object in one service, which may affect other services.

Describe the solution you'd like

Adding readonly config type directly to framework could prevent config mutation errors on compile time and reduce amount of boilerplate code which is similar across projects.

API

There are 3 possible APIs for this:

  1. Making ConfigType infer readonly config type by default. I think it is the best solution, however it introduces a BREAKING CHANGE

  2. Changing ConfigType signature:

    // Before
    type ConfigType<T extends (...args: any) => any> = ...
    
    // After
    type ConfigType<T extends (...args: any) => any, IsReadonly extends boolean = false> = ...

    This solution doesn't affect existing codebases, however it adds possibility to make ConfigType to infer readonly config type. The API is similar to WasValidated param in ConfigService. I don't like this solution as adding extra param is a bit confusing in my opinion.

  3. Introducing a new ReadonlyConfigType type. In my opinion new type is more expressive than the second solution.

Implementation

There are 2 ways to implement readonly config type:

  1. Naive ReadonlyDeep. This implementation is limited as it doesn't narrow arrays for tuples for example and doesn't work with arrays of objects.

    type ReadonlyDeep<T> = {
      readonly [P in keyof T]: T[P] extends Record<string, unknown>
        ? ReadonlyDeep<T[P]>
        : T[P];
    };
  2. Recursive ReadonlyDeep see type-fest ReadonlyDeep implementation.

What is the motivation / use case for changing the behavior?

.env

PORT=3000

src/config.ts

...

export default registerAs("config", () => ({
  port: Number(process.env.PORT),
}));

src/app.service.ts

...

@Injectable()
export class AppService {
  constructor(
    @Inject(config.KEY) private readonly configuration: ConfigType<typeof config>
  ) {}

  mutateConfig() {
    // port now is globally equal to 3001
    // No ts errors
    this.configuration.port = 3001;
  }
}

Adding as const to config registration doesn't solve the issue as ConfigType still infers as a mutable config type:

export default registerAs("config", () => ({
  port: Number(process.env.PORT),
} as const));

For now the only way to address this issue is to manually narrow inferred config type to readonly:

type ReadonlyDeep<T> = ...

type Config = ReadonlyDeep<ConfigType<typeof config>>;
@kamilmysliwiec
Copy link
Member

Making ConfigType infer readonly config type by default. I think it is the best solution, however it introduces a BREAKING CHANGE

Sounds great!
Would you like to create a PR for this issue? We can wait with merging it till the next major release (due to a breaking change)

@yura2100
Copy link
Author

Sounds great! Would you like to create a PR for this issue? We can wait with merging it till the next major release (due to a breaking change)

Sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants