Use parseEnv instead of dotenv and avoid side effects#1947
Conversation
🦋 Changeset detectedLatest commit: f126035 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #1947 +/- ##
==========================================
- Coverage 82.61% 81.90% -0.72%
==========================================
Files 54 54
Lines 2422 2442 +20
Branches 718 721 +3
==========================================
- Hits 2001 2000 -1
- Misses 379 400 +21
Partials 42 42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "@changesets/changelog-github": patch | ||
| --- | ||
|
|
||
| Use `loadEnvFile` instead of `dotenv` to load the `.env` file |
There was a problem hiding this comment.
| Use `loadEnvFile` instead of `dotenv` to load the `.env` file | |
| Replaced `dotenv` with `loadEnvFile` for loading `.env` files |
i think generally changesets should use past tense, as they are a log of what has changed in a package, compared to commit descriptions that use present tense as they describe what a commit does or will do to a repository
There was a problem hiding this comment.
I've always used present tense here as they work like git commits to me. It depends how you read it but they could also describe what a specific version does. I don't think we need to standardize the format for now though.
There was a problem hiding this comment.
it would be nice to have one before the v3 release to make it look nicer though :)
There was a problem hiding this comment.
we should go through the final changesets/changelog updates manually before releasing final v3 anyway, so we should be able to adjust the lanaguage then
| @@ -1,8 +1,15 @@ | |||
| import { loadEnvFile } from "node:process"; | |||
There was a problem hiding this comment.
I have a semi-strong beef with unexpected side effects. This isn't new here, of course, but given we touch this now - what do you think about using util.parseEnv instead of this? It would be a couple of LOC more but I think it would be nice to avoid the side-effect and do this lazily.
There was a problem hiding this comment.
that's true, it shouldn't effect the rest of the code
There was a problem hiding this comment.
I pushed out that change, let me know what do you think about this.
There was a problem hiding this comment.
Yeah I agree too. Personally I wouldn't want a package to implicitly read a .env file altogether though but we can think of the more breaking change in the future. The change now shouldn't be a big deal.
There was a problem hiding this comment.
Yeah, I think .env support is kinda esoteric here. OTOH, it's pretty cheap so 🤷 but I doubt that many people rely on this. It was introduced when there was only one env variable supported too (GITHUB_TOKEN)
bluwy
changed the title
|
@Andarist Should we cache and only load the env files once? From the code paths it looks like it could be called repeatedly |
|
ye, that's a good idea |
3 participants
https://nodejs.org/api/process.html#processloadenvfilepath exists since the recent required nodejs version bump. We no longer need to use and upgrade the dotenv dependency.
close #1931