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.

Sign up for GitHub

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

Jump to bottom

[BREAKING] Fix #155 | Upgrade jest & node version #172

Closed
wants to merge 1 commit into from
Closed

[BREAKING] Fix #155 | Upgrade jest & node version #172

wants to merge 1 commit into from

Conversation

antoine-pous
Copy link
Contributor

@antoine-pous antoine-pous commented Oct 18, 2021 •
edited

Changelog:

What's wrong:

Currently Jest encounter an issue I've never seen before, it's probably related to the fact I'm using node 16 but maybe not, I've no time to dig into.

Breaking change

This PR introduce many breaking change, then envalid version must be updated consequently.

  • default and devDefault are now exclusively optionnal string ?: string. As explained on default and default:Dev should be of type string #155 issue, env var are only string and parsing always the same kind of input is the best way to avoid typing and runtime issues. It ensure the consistency of MakeValidator and developpers can output any type of value anyway

  • url() now return URL instead of a string, then we can manipulate it the right way manually, If we need to use it as string the module provide everything we need.

  • str now validate if the string is empty by trimming spaces instead of just checking the type, as input can only be string the type check is now useless.

  • All validators have been updated to follow the new way to make validators.

Using it before merging

I'm actually using this PR in production environment and will update it if I found any issue related to those changes. For those who would like to test it and give an advice here's the way to install this version:

  • using yarn: yarn add https://github.com/antoine-pous/envalid.git
  • using npm: npm i https://github.com/antoine-pous/envalid

@antoine-pous antoine-pous changed the title [BREAKING][WIP] Fix #155 | Upgrade jest & node version [BREAKING] Fix #155 | Upgrade jest & node version Oct 23, 2021
@antoine-pous
Copy link
Contributor Author

@af can you review those changes and tell me if you see something to change before merging?

@af
Copy link
Owner

af commented Oct 24, 2021

Hi @antoine-pous, I'm in the middle of moving and haven't had a chance to take a look yet, sorry! I'll try and get some feedback to you in the next week

@af
Copy link
Owner

af commented Oct 31, 2021 •
edited

Alright, sorry it took a while but I finally had a chance to dig in today.

First off, a general dependency updates pass has been merged, and I can confirm that node 16 now works on latest main (both locally and on CI). Thanks for flagging that!

For the other changes here– I think they're a bit too radical, and removing the type param from Spec removes the ability to specify a subtype, which some people rely on (eg FOO: str<'bar' | 'baz'>())

One of the big mistakes I made with this library (very evident in the discussion in #102) is making it unclear whether default is a default input or output (post-validation) value. My preference is for the latter interpretation, as providing numeric/boolean default values as strings feels icky to me, and the latter has a nice type inference benefit, eg union types can now be inferred in the following example rather than the more broad string and number types:

  const spec = cleanEnv({ NODE_ENV: 'test' }, {
    NODE_ENV: str({ choices: ['production', 'test', 'development'] }),
    PORT: num({ choices: [80, 3000, 8080] })
  })

With that in mind, I'll investigate to see if the default values can be considered "clean" and passed through without validation, as that should remove the inconsistency.

If you need a default input value, you can always add it in before being passed to envalid, ie envInput = { FOO: 'defaultval', ...process.env, }

@af af closed this Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

default and default:Dev should be of type string
2 participants
@antoine-pous @af