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

feat: Introduces JSON type to non-public env variables #757

Merged
merged 6 commits into from
Sep 8, 2023
Merged

feat: Introduces JSON type to non-public env variables #757

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba07cd2
Introduces JSON type to non-public env
sleekslush Sep 7, 2023
4e77ee9
Merge branch 'main' into env-json-directive
louisgv Sep 8, 2023
9ca5bab
Uses json(...) as the directive
sleekslush Sep 8, 2023
7eeb075
Merge branch 'env-json-directive' of github.com:sleekslush/plasmo int…
sleekslush Sep 8, 2023
47059a1
Uses dotenv-expand for var interpolation
sleekslush Sep 8, 2023
c51a844
Merge branch 'main' into env-json-directive
louisgv Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cli/plasmo/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
"chalk": "5.3.0",
"change-case": "4.1.2",
"dotenv": "16.3.1",
"dotenv-expand": "10.0.0",
"events": "3.3.0",
"fast-glob": "3.3.1",
"fflate": "0.8.0",
Expand Down
39 changes: 22 additions & 17 deletions cli/plasmo/src/features/env/env-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { readFile } from "fs/promises"
import { resolve } from "path"
import { constantCase } from "change-case"
import dotenv from "dotenv"
import { expand as dotenvExpand } from "dotenv-expand"

import { isFile, isReadable } from "@plasmo/utils/fs"
import { eLog, iLog, vLog } from "@plasmo/utils/logging"
Expand All @@ -16,6 +15,7 @@ type LoadedEnvFiles = Array<{
contents: string
}>

const JSON_DIRECTIVE_REGEX = /^\s*~/
sleekslush marked this conversation as resolved.
Show resolved Hide resolved
export const INTERNAL_ENV_PREFIX = "PLASMO_"
export const PUBLIC_ENV_PREFIX = "PLASMO_PUBLIC_"

Expand Down Expand Up @@ -50,22 +50,17 @@ function cascadeEnv(loadedEnvFiles: LoadedEnvFiles) {
for (const { contents, name } of loadedEnvFiles) {
try {
envFileSet.add(name)
const result = dotenvExpand({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found no real benefit to the side-effects introduced by dotenvExpand, since there is explicit process.env var assignment in the iteration of the values from the .env files. If I'm mistaken, let me know, but leaving it in also conflicts with the ability to introduce this feature since it implicitly hydrates process.env

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Gathering my thought) the main idea with dotenvExpand is to have this:

PASSWORD="s1mpl3"
DB_PASS="prefix-$PASSWORD"

And the reason we want that is to have the same behavior as nextjs and other frameworks with built-in env parsing.

but leaving it in also conflicts with the ability to introduce this feature since it implicitly hydrates process.env

Can we run the expansion at the end of this loop? (on the parsed object)?

Perhaps that might be best. I.e on line 72, we'd do:

return dotenvExpand.expand({ parsed })

That way, we can hydrate the key for the JSON value manually first, then expand once we know everything's in-place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, that explanation makes more sense. I'll play around with it a bit more and make sure that still works

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think my latest commit resolves this, but maybe you have some specific use cases you can test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superb - works for me! Tested on some variant with the dnr as well!

parsed: dotenv.parse(contents)
})

if (!!result.parsed) {
vLog(`Loaded env from ${name}`)
const resultData = result.parsed || {}

for (const envKey of Object.keys(resultData)) {
if (typeof parsed[envKey] === "undefined") {
parsed[envKey] = resultData[envKey]

// Pass through internal env variables
if (envKey.startsWith(INTERNAL_ENV_PREFIX)) {
process.env[envKey] = resultData[envKey]
}

vLog(`Loaded env from ${name}`)
const resultData: dotenv.DotenvParseOutput = dotenv.parse(contents) || {}

for (const [envKey, envValue] of Object.entries(resultData)) {
if (typeof parsed[envKey] === "undefined") {
parsed[envKey] = maybeParseJSON(envValue)

// Pass through internal env variables
if (envKey.startsWith(INTERNAL_ENV_PREFIX)) {
process.env[envKey] = envValue
}
}
}
Expand All @@ -77,6 +72,16 @@ function cascadeEnv(loadedEnvFiles: LoadedEnvFiles) {
return parsed
}

function isJSONDirective(value: string): boolean {
return JSON_DIRECTIVE_REGEX.test(value);
}

function maybeParseJSON(value: string): any {
return isJSONDirective(value)
? JSON.parse(value.replace(JSON_DIRECTIVE_REGEX, ""))
: value;
}

export const getEnvFileNames = () => {
const nodeEnv = process.env.NODE_ENV
const flagMap = getFlagMap()
Expand Down
8 changes: 0 additions & 8 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.