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

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

merged 6 commits into from
Sep 8, 2023

Conversation

sleekslush
Copy link
Contributor

@sleekslush sleekslush commented Sep 7, 2023

Details

Note: I'm open to conversation on this idea, so please let me know your thoughts and whether something like this would be useful. Also, would be curious to know if any alternative solutions might be preferred. Thanks!

In non-public env (anything using $SOMETHING that isn't explicitly accessing process.env.SOMETHING), this adds a special syntax to the environment variable values to treat the value as JSON-parseable instead of just a string.

An example use case is in a .env and package.json:

EXTERNALLY_CONNECTABLE_MATCHES=`json([
    "*://*.domain1.com/*",
    "*://*.domain2.com/*"
])`
{
    "manifest": {
        "externally_connectable": {
            "matches": "$EXTERNALLY_CONNECTABLE_MATCHES"
        }
    }
}

The resulting manifest.json will look like:

{
    "manifest": {
        "externally_connectable": {
            "matches": [
                "*://*.domain1.com/*",
                "*://*.domain2.com/*"
            ]
        }
    }
}

It's important to note that this can only be used for variables that won't be access via process.env, since those values are coerced as string by node.

Code of Conduct

  • I agree to follow this project's Code of Conduct
  • I agree to license this contribution under the MIT LICENSE
  • I checked the current PR for duplication.

Contacts

  • (OPTIONAL) Discord ID: filthytone

If your PR is accepted, we will award you with the Contributor role on Discord server.

To join the server, visit: https://www.plasmo.com/s/d

@@ -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!

Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

Some nits, but overall loving the idea!

@@ -50,22 +50,17 @@ function cascadeEnv(loadedEnvFiles: LoadedEnvFiles) {
for (const { contents, name } of loadedEnvFiles) {
try {
envFileSet.add(name)
const result = dotenvExpand({
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?

cli/plasmo/src/features/env/env-config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@louisgv louisgv added this pull request to the merge queue Sep 8, 2023
Merged via the queue into PlasmoHQ:main with commit 377e0ab Sep 8, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: TODO - PR WELCOME
Development

Successfully merging this pull request may close these issues.

2 participants