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

Incorrect types for values using environment variables #82

Open
DonIsaac opened this issue Jan 15, 2023 · 3 comments
Open

Incorrect types for values using environment variables #82

DonIsaac opened this issue Jan 15, 2023 · 3 comments

Comments

@DonIsaac
Copy link
Contributor

Describe the bug:
Types generated for properties using environment variables are incorrect. Environment variable configs should use string | undefined as that's what @types/node uses for environment variables. However, during interface generation, each environment variable is either string or undefined, depending on whether or not it's defined at the time. When it is, the property will have a type of string. Otherwise, it will have a type of undefined.

Suspected Cause

Type generation follows the following process:

  1. Load configs for the appropriate environment (based on values for relevant environment variables)
  2. Merge them together
  3. Resolve values for config properties using environment variables
  4. Pass the resulting object to json-to-ts

e.g.

// config/default.json
{
  "foo": "bar"
}
// config/env/dev.json
{
  "foo": "@@FOO"
}

When process.env.FOO is undefined, mergeAllConfigs results in the final object:

{
  "foo": undefined
}

json-to-ts, seeing undefined, resolves the type for that particular config to undefined.

Possible Solutions

Solution 1: use null instead of undefined (worse solution)

json-to-ts handles null values, but not undefined values, by making their types optional and any. The below test from their codebase demonstrates this behavior.

  it("should work with multiple key words and optional fields", function() {
    const json = {
      "hello world": null
    };

    const expected = `
interface RootObject {
  'hello world'?: any;
}`;
    const actual = JsonToTS(json).pop();
    assert.strictEqual(expected.trim(), actual.trim());
  });

This is better than undefined as this typing is correct, but it's not very specific.

Solution 2: use optional property name syntax

json-to-ts has undocumented behavior for creating optional types based on property names. Its implemented in this function. Properties ending in --? will have optional types, and the --? suffix gets removed from the resulting type. e.g.

{
  "foo--?": "foo"
}

Turns into

interface RootObject {
  foo?: string
}

We could use this to our advantage. During type generation, we could insert dummy values into values that use environment variables and add the --? suffix to the key. This would result in the correct type. The downside is that this solution relies on undocumented behavior.

Steps To Reproduce:

Create a bare-bones default config

// config/default.json
{
  "foo": "@@FOO"
}

Do not set a value for FOO in your terminal. Then, run the CLI. The following type declaration will be created:

// config/Config.d.ts
/* tslint:disable */
/* eslint-disable */
declare module "node-config-ts" {
  interface IConfig {
    foo: undefined
  }
  export const config: Config
  export type Config = IConfig
}

Next, set foo and run the CLI again.

export FOO=foo
node ./bin/cli

The following type declaration will be created:

// config/Config.d.ts
/* tslint:disable */
/* eslint-disable */
declare module "node-config-ts" {
  interface IConfig {
    foo: string
  }
  export const config: Config
  export type Config = IConfig
}

Expected behavior:

The type declaration below should be generated:

// config/Config.d.ts
/* tslint:disable */
/* eslint-disable */
declare module "node-config-ts" {
  interface IConfig {
    foo: string | undefined
  }
  export const config: Config
  export type Config = IConfig
}

Typescript Version:
Tested on 3.7.5 and 4.9.4. Should not affect the behavior.

node-config-ts version:
3.1.0 and 3.2.0

@DonIsaac
Copy link
Contributor Author

As a sidenote: json-to-ts appears to be abandoned. There hasn't been a code change in over 7 months. See this Snyk Advisor report

@tusharmath
Copy link
Owner

I would like to propose something completely different here —
We should create types first and then have readers that read data from various sources (files, env etc.) and merge them together to create a value of the expected type.

@tusharmath
Copy link
Owner

tusharmath commented Jan 20, 2023

It would work something like this

import {config, Source} from 'node-config-ts'

type Color = "RED" | "BLUE" | "GREEN"

type Config = {
  port: number,
  color: Color
}


// Generates a config with some default values
const auto: Source<Config> = config.automatic.as<Config>()

// Reads from default.json
const default: Source<Config> = config.file("./default.json").as<Config>()

// Reads from production.json
const production: Source<Config> = config.file("./production.json").as<Config>()

// Read from env variables
const env: Source<Config> = config.env.as<Config>()

// Merge all the configurations
const merged: Source<Config> = auto.orElse(default).orElse(production).orElse(env)

console.log(await merged.generate()) // Will print the final config

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