The Wayback Machine - https://web.archive.org/web/20201223081943/https://github.com/af/envalid/issues/115
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

Migrate codebase to TypeScript #115

Open
af opened this issue Dec 31, 2019 · 15 comments
Open

Migrate codebase to TypeScript #115

af opened this issue Dec 31, 2019 · 15 comments

Comments

@af
Copy link
Owner

@af af commented Dec 31, 2019

I was a little slow jumping on the TS bandwagon but I'm a full convert now. I would like to convert the full codebase to TS sometime in the near future, but haven't had the time to do so yet. Creating this as a tracking issue and advertising the intent, in case someone wants to do this before I get around to it :)

@KATT
Copy link

@KATT KATT commented Sep 16, 2020

I'm keen to have a go at this!

@KATT
Copy link

@KATT KATT commented Sep 16, 2020

But I might approach it a bit differently as I use envalid in the browser a lot. I'd like it to be:

  • Strict by default
  • Have no dependencies
  • No useage of proxy
@SimenB
Copy link
Collaborator

@SimenB SimenB commented Sep 16, 2020

That's a separate discussion from migrating to TS - ideally a TS migration should have no logic changes

@KATT
Copy link

@KATT KATT commented Sep 16, 2020

Yeah, you're right, I snowballed a few different discussions in there.

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Sep 16, 2020

That said, would love a PR migrating to TS and an issue discussing potential changes 🙂

@KATT
Copy link

@KATT KATT commented Sep 16, 2020

What's your opinion on using tsdx to reduce boilerplate?

@SimenB
Copy link
Collaborator

@SimenB SimenB commented Sep 16, 2020

Up to @af, but if it helps us get up and running we can always rip it out later

@af
Copy link
Owner Author

@af af commented Sep 16, 2020

This sounds great! I agree with SimenB that we should limit the breaking changes as part of the conversion. Ideally the conversion could happen without requiring a major version bump. Personally I agree the lib should be strict by default, but we should move that to a separate discussion.

On the dependencies side, this lib was never initially intended for browser usage, so it's not optimized for that case dependency-wise. According to bundlephobia something like 50% of the bundle size comes from the chalk dependency, which could easily be replaced by creating a couple small helper functions to add the ANSI color codes.

@KATT
Copy link

@KATT KATT commented Sep 16, 2020

Hey again! So, it was easier for me to start from scratch and add things gradually than to get it working here so it resulted in a new repo as you can see here: https://github.com/KATT/envsafe

Which then also led me to change some things to how I'd like it 😬 ... overall it's very similar but kinda slimmed down the API-surface, it's all in TypeScript and no third party deps added (yet).

What I'm "missing" right now is:

  • host-support
  • the testOnly-thing
  • non-strict mode & the proxy-stuff that throws error
  • chalk

How do you think we should proceed? I have a feeling like the changes are too major for you to want it in as it is, so maybe we keep it as two sister projects?

@KATT
Copy link

@KATT KATT commented Sep 16, 2020

I'm very happy to pass this back into envalid and burn that project if you like it, but I don't know if you like the changes, want to do a major version, etc.

@af
Copy link
Owner Author

@af af commented Sep 18, 2020

TBH I like the changes for the most part. I'm not sure how much people use those missing features but the only things I would miss personally are:

  • colorized CLI output (chalk is overkill for this anyways, but colors are nice for readability in the terminal)
  • errors from the Proxy (could maybe create extension points that make this possible in "userland"?)

I'll take a closer look this weekend but I'm intrigued by the idea of this being a v7. Curious if any of those features are a "dealbreaker" for anyone.

@KATT
Copy link

@KATT KATT commented Sep 18, 2020

Great to hear! I'd prefer to have one-package-to-rule-them-all rather than adding more fragmentation.

  • I've added the proxy, but I already want to refactor it to just expose freezeObject so the logic is not always included in the bundle when using it in the browser
  • If there's a way (that doesn't take a day of configuration) of conditionally including chalk depending if node/browser it'd be nice. Colors are nice indeed!

Other notable changes:

  • The first arg is now the options and env is optional in the options obj with process.env as default.
  • Added input to help with environments where you can't iterate on the process.env - but I'm working on a webpack plugin which could replace it and also make the build fail at compile time rather than at runtime.
  • I removed process.exit(1) and simply throw the error as it gives better error messages in CI - exit propagates exit(1) anyway

Removed:

  • host
  • No built-in dotenv-stuff. Can just require('dotenv') before calling envsafe()
  • transformer - I don't see the point. If you wanna transform the object after creating the env, just do it, I don't see why an API is needed for it.
  • No second arg on makeValidator - didn't see the point.
@af
Copy link
Owner Author

@af af commented Sep 28, 2020

Update: I finally had some time to check this out over this last weekend. Thanks @KATT, looks great and while I think overall it's a big step forward, to reduce churn for existing users I think we should roll the changes into the existing API. My WIP isn't in a reviewable state yet, but hopefully will be in a couple of days. Wanted to get feedback on the following plan though:

Major changes I think we should incorporate:

  • Rewrite in TypeScript (obviously)
  • Use tsdx to simplify tooling, including migrating tests to jest (I still like painless but it's abandoned 😢 )
  • Remove all runtime dependencies. The value they bring right now is not (IMO) worth the cost for client-side users (who are more numerous than I ever anticipated). As you mentioned, dotenv can just be used before passing the env object into envalid. The other deps are really just providing minor conveniences, and being dependency-free would be great
  • Introduce the concept of "middlewares", which basically do the same thing as transformer does now. The proxy and isDev/isProd/etc properties can probably be re-implemented as middlewares to make things more modular
  • Turn on strict behavior by default and remove the option. If you want to preserve existing non-strict behavior, you can turn off the proxy middleware and do something like const output = { ...process.env, ...cleanedEnvFromEnvalid }

However I don't want to do a full rewrite of the API; the churn would probably be too annoying for existing users. That means:

  • Keep all existing validators, including host. host and ip rely on an existing package currently, but a simplified version can be rolled into envalid. If a user wants more exhaustive checking, they can use a custom validator to recreate the v6 behavior
  • Keep as much of the existing test suite as possible
  • Minimize overall api changes unless they're unavoidably tied to the benefits above
@KATT
Copy link

@KATT KATT commented Sep 29, 2020

Sounds like good plan! I'm happy to take a look at your WIP whenever.

@af af mentioned this issue Oct 5, 2020
1 of 3 tasks complete
@af
Copy link
Owner Author

@af af commented Oct 5, 2020

@KATT Just pushed up a WIP PR, would love your feedback 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.