fix(laconia-config): register integer and float converters to EnvVarC…

[reestolonio]

• Added IntegerConfigConverter and FloatConfigConverter in EnvVarConfigFactory

[hugosenari]

@reestolonio to me is ok, thank you. 🎉

Inviting a more experienced dev (@geoffdutton) to review.

[geoffdutton]

@reestolonio Nice catch! I think I'm the one that made the integer and float converters, so you fixed my bug--thanks ;)

The only thing I'd say is add a test that the function exports.envVarInstances returns an EnvVarConfigFactory with all of the converters.

[geoffdutton]

Add test that expects the .converters prop has the keys ['boolean', 'integer', 'float', 'ssm', 's3', 'secretsManager']

[reestolonio]

Add test that expects the .converters prop has the keys ['boolean', 'integer', 'float', 'ssm', 's3', 'secretsManager']

Hi @geoffdutton , was checking way to test the .converters. Unfortunately, there is no way to test through index.spec.js. This test cannot access the instance of EnvVarConfigFactory, therefore, cannot assert .converters. Any suggestion?

Also, while this kind of test is good verify the current behavior, it does not verify if new converter is added to the config.

[geoffdutton]

@reestolonio You are correct it doesn't verify when new converters are added. I view it as more of a preemptive regression test when refactoring.

Let me look through it a bit more.

In the meantime, is this something you'd like to see get released and deployed soon?

Edit: @hugosenari and @ceilfors I think it's fine to release as is

[geoffdutton]

Ideally we figure out a "sanity check" test at some point, but otherwise looks good to me.

[reestolonio]

@geoffdutton , yeah if possible. I have a code in development waiting for this 😄 . But I respect your release schedule. I can use parseXXX for now.

[geoffdutton]

@reestolonio I believe @ceilfors is the one that has to publish to NPM.

[hugosenari]

We could use it in Acceptance Tests, will not verify when new converters are added, but will check any changes that break 'contract' (API)

[geoffdutton]

Or can we define the type definition that requires all of the converters to instantiate an EnvVarConfigFactory object? I'm not too familiar with Typescript.

[reestolonio]

Validating the 'contract'-breaking changes can also be done at package-/unit- level. Sample unit test below would satisfy the requirement of @hugosenari in above comment

The idea is to test if all converters are loaded by default to the factory. Other tests in laconfig-config don't have this validation.

describe("@laconia/config", () => {
  describe("#envVarInstances", () => {
    it.only("resolves all config objects", async () => {
      const $ssm = // mock
      const $s3 = // mock
      const $secretsManager = // mock
      
      // !!! developer will be required to add entry here if new converter is added.
      const env = {
        LACONIA_CONFIG_TEST_BOOL: 'boolean:true',
        LACONIA_CONFIG_TEST_INTEGER: 'integer:1',
        LACONIA_CONFIG_TEST_SSM: 'ssm:/path/to/parameter',
        // add more variables here
      }
      const instances = await config.envVarInstances()({ env, $ssm, $s3, $secretsManager });
      expect(instances).toHaveProperty('testBool', true)
      expect(instances).toHaveProperty('testInteger', 1)
      // add more validations to all the objects created  by config
    })
  });
});

[ceilfors]

@all-contributors please add @reestolonio for code

[allcontributors]

@ceilfors

I've put up a pull request to add @reestolonio! 🎉

[ceilfors]

Apologies for my slow reply, for some reason GitHub has no longer send a notification to my email! Still figuring out how to fix this

Thanks for the PR! I'm going to release this now!

[ceilfors]

Released at v1.10.0 🎉