sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.98k stars 157 forks source link

`process.env` cannot be decoded/transformed because it's not considered as a plain object #626

Closed antmarot closed 1 year ago

antmarot commented 1 year ago

I ran into this while trying to define schemas for my environment variables, with some Type.Transform to properly convert some primitive types like booleans and numbers.

My unit tests were all working as long as I gave a mocked env object:

myDecodeFunction<SomeEnvSchema>({ MY_VAR: 'true' });
// returns { MY_VAR: true } — CORRECT

But as soon as I tried to use process.env, no transformations happened:

myDecodeFunction<SomeEnvSchema>(process.env);
// returns { MY_VAR: 'true', ... } — INCORRECT

From my investigation, this would happen because process.env returns false from IsPlainObject and is therefore returned as it is, without any transformations. This would be due to the .constructor of process.env that is an anonymous function, and as a consequence its name cannot be Object.

Note that a workaround is therefore to create an actual object copy of process.env:

myDecodeFunction<SomeEnvSchema>({ ...process.env });
// returns { MY_VAR: true, ... } — CORRECT

Is there any way to improve Is(Plain)Object with respect to cases like process.env? On another note, shouldn't Value.Decode throw instead of defaulting to values that do not match its ReturnType?

sinclairzx81 commented 1 year ago

@antmarot Hi,

The following should parse environment variables.

TypeScript Link Here

import { Type, TObject, StaticDecode } from '@sinclair/typebox'
import { Value } from '@sinclair/typebox/value'

// ------------------------------------------------------------------
// Parse Env
// ------------------------------------------------------------------
export function parseEnv<T extends TObject>(schema: T, value: unknown): StaticDecode<T> {
  // check value is valid, throw if invalid
  if(!Value.Check(schema, value)) throw Error(Value.Errors(schema, value).First()!.message)
  // cast value in to mapped schematic, optionally remove additional properties.
  const casted = Value.Cast({ ...schema, additionalProperties: false }, value)
  // run the decode on the casted value
  return Value.Decode(schema, casted)
}

// ------------------------------------------------------------------
// Usage
// ------------------------------------------------------------------
export const NumericString = Type.Transform(Type.String({ pattern: '^[+-]?([0-9]*[.])?[0-9]+$' /* ensure numeric string */ }))
  .Decode(value => parseFloat(value))
  .Encode(value => value.toString())

const Environment = Type.Object({ // some variables (replace with expected)
  NUMBER_OF_PROCESSORS: NumericString,
  PROCESSOR_LEVEL: NumericString,
})

const env = parseEnv(Environment, process.env)

console.log(env)

IsPlainObject

From my investigation, this would happen because process.env returns false from IsPlainObject and is therefore returned as it is, without any transformations. This would be due to the .constructor of process.env that is an anonymous function, and as a consequence its name cannot be Object.

I might need to take another look at this. The intent here is that decoding should only decode for objects that are not instances of classes (as classes may have private fields that cannot be decoded into a valid instance of that class (which is a partial requirement for Encode)). If the value (process.env in this case) happens to be a class instance, expanding enumerable properties via the ... operator may need to remain as a requirement (but might look into this again in the next update). There's a few complexities to consider here.

For now, give the above parseEnv a try. Let me know how you go Cheers S

antmarot commented 1 year ago

@sinclairzx81 Thank you very much for your reply!

I did some tests with the parseEnv you provided and it works, as long as we set additionalProperties to false (as you did). I believe the reason it fails when we set additionalProperties to true is due to this Check — as it passes when we allow extra props, the value is returned as is and we do not build a true plain object copy as it's done in the rest of that function. The value passed down to Decode is therefore still the process.env value itself and is returned as is once again since it is not considered a plain object.

I tried to find some info about whether or not process.env is the instance of a class or a plain object, and while it is not clearly stated anywhere, it is however mentioned in the official doc that:

Assigning a property on process.env will implicitly convert the value to a string.

which would indicate that we're not dealing with a simple object primitive, but maybe some class or maybe some proxy, ...

I'm not sure what could be done to detect/handle cases like this, at least not without being too adhoc or too leniant, so I think this ticket can be closed. (edit: maybe Cast could always copy objects?)

Thank you for this great library, which is IMO the best by far in that domain. Cheers

sinclairzx81 commented 1 year ago

@antmarot Hi, thanks.

I'm not sure what could be done to detect/handle cases like this, at least not without being too adhoc or too leniant, so I think this ticket can be closed. (edit: maybe Cast could always copy objects?)

Yeah, I'll take another look at this on the next update but will likely keep things as they are for now. I think removing the IsPlainObject requirement might be ok and would permit more exotic objects being passed in, but there are complexities around ensuring the encoded form is correct (as transforms can nest within other transforms, and it's difficult to ensure a nested transform conforms to some constructor or proxied instance), will need to give this some thought.

Will close off this issue for now, but will mark for review on the next round of updates All the best! S