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

Fix .env.example template #184

Closed
wants to merge 2 commits into from
Closed

Fix .env.example template #184

wants to merge 2 commits into from

Conversation

rin-st
Copy link
Member

@rin-st rin-st commented Dec 26, 2024

Currently, we can't use multiple values since we take stringifyed version even if we pass an array.

For example, if I pass in .env.example.args.mjs file

export const additionalVars = ["SOME_ENV_VAR=", "SOME_ENV_VAR_2="];

I'm getting this in .env.example as a result:

...
SOME_ENV_VAR=,SOME_ENV_VAR_2=

PR fixes it, so result is

...
SOME_ENV_VAR=
SOME_ENV_VAR_2=

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Umm @rin-st I think you can pass the args like this:

.env.example.args.mjs

export const additionalVars = `
# Some descripiton about SOME_ENV_VAR on how to get it
SOME_ENV_VAR=hello

# Some descripiton about SOME_ENV_VAR_2 on how to get it
SOME_ENV_VAR_2=world
`

Just tried it and seems to work but maybe I am missing something?

@carletex
Copy link
Member

I think what Shiv is mentioning works.

But (I guess related to #170 #180) this also opens some questions:

What is the preferred method to pass args? Should we do structured data where we can (like the array in Rinat's example), and only template literals when required? Should we allow both? I think we already talked about this in a previous issue/PR.

@rin-st
Copy link
Member Author

rin-st commented Dec 27, 2024

As I understand we decided to allow using objects/arrays when possible?

But for this since there is not so much logic in template and passing an array export const additionalVars = ["SOME_ENV_VAR=", "SOME_ENV_VAR_2="] looking a bit weird we can leave it as is and just pass a string

@carletex
Copy link
Member

As I understand we decided to allow using objects/arrays when possible?

Not sure, but I think the key is to be consistent!

Now we are also allowing both methods, but 1 is failing (generating something that's wrong)

@technophile-04
Copy link
Collaborator

technophile-04 commented Dec 27, 2024

As I understand we decided to allow using objects/arrays when possible?

I think this was in context when assigning some values to variables or object keys (like hardhatConfig or lightTheme in tailwind config)? For the logic inside file as a whole I think it should be template literal since this allows more customization (example in this case having comments above env variables)?

Agree that we should be consistent but also I think types of what args can be passed will be discovered by scaffold-eth/create-eth-extensions#46 this branch? So we go with most sensible option?

@carletex
Copy link
Member

Agree that we should be consistent but also I think types of what args can be passed will be discovered by scaffold-eth/create-eth-extensions#46 this branch? So we go with most sensible option?

Yes, the branch is great. But we shouldn't say "hey, it doesn't matter if we are not consistent because you can look it up in the branch". Being consistent is better for the extensions devs but also for us, in every sense I think.

e.g:

I think this was in context when assigning some values to variables or object keys (like hardhatConfig or lightTheme in tailwind config)? For the logic inside file as a whole I think it should be template literal since this allows more customization (example in this case having comments above env variables)?

We can agree on this or not... but this is consistent!

Sorry, I think I forget some of these conversations haha. Maybe we can make it more explicit in the future in the docs (after 170/180)

@rin-st
Copy link
Member Author

rin-st commented Dec 27, 2024

For the logic inside the file as a whole, I think it should be template literal since this allows more customization (for example, having comments above env variables in this case).

Agree with Shiv here, but in that case we need to prevent passing arrays, because I also agree with Carlos below )

Now we are also allowing both methods, but 1 is failing (generating something that's wrong)


Sorry, I think I forget some of these conversations haha. Maybe we can make it more explicit in the future in the docs (after 170/180)

100% we need it to understand better what "consistency" is in terms of templates/args. Basically, we decide what the right parameters are. Maybe sometimes we just need to add comments to template files, or args files of example extension

But we shouldn't say "hey, it doesn't matter if we are not consistent because you can look it up in the branch".

Maybe for better DX we can also add args typecheck like

If (type of default argument !== type of passed argument) then throw an error "hey, wrong argument type here, see default param and example extension"

so we could be inconsistent sometimes when we need too, e.g. in this .env.example case and devs will figure it out explicitly and earlier


returning to this PR, tending to merge it for now just because of

Now we are also allowing both methods, but 1 is failing (generating something that's wrong)

But if you think it's not worth it, we can close it

@carletex
Copy link
Member

returning to this PR, tending to merge it for now just because of

Now we are also allowing both methods, but 1 is failing (generating something that's wrong)

But if you think it's not worth it, we can close it

But it's up to you all! No strong opinions here.

If (type of default argument !== type of passed argument) then throw an error "hey, wrong argument type here, see default param and example extension"

I think this makes a lot of sense too.

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

If (type of default argument !== type of passed argument) then throw an error "hey, wrong argument type here, see default param and example extension"

so we could be inconsistent sometimes when we need too, e.g. in this .env.example case and devs will figure it out explicitly and earlier

Love this idea! Make sense and we should do it!

So then we close this PR and allow only directly passing of strings? If people pass array we throw error?

I mean we could do vice versa too only allow array and block strings but it will not that ergonomic to add comments above env vars?


Umm also I feel this is gonna happen in future too like what's intuitive/ergonomic for Rinat (in this case passing array) might be not be that intuitive for me.

Maybe it's better we layout some rules on what type of args should we go with based on certain conditions? Like:

  • Using objects / arrays when assigning some values to variables or object keys (like hardhatConfig or lightTheme in tailwind config)
  • When there is dangling args variable in file we only accept string literals?

maybe this what you all meant by being consistent (having proper rules?) lol sorry maybe we discussed this already in some other discussion or somewhere. Gonna go through discussions again.

@@ -14,7 +14,7 @@ const contents = ({ additionalVars }) => {
# More info: https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables
NEXT_PUBLIC_ALCHEMY_API_KEY=
NEXT_PUBLIC_WALLET_CONNECT_PROJECT_ID=
${additionalVars.join("\n")}
${additionalVars[0].join("\n")}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add check :

${additionalVars[0] ? additionalVars[0]?.join("\n") : ''}

Other wise it fails in normal yarn cli --skip command.

Screenshot 2025-01-01 at 3 02 22 PM

@carletex
Copy link
Member

carletex commented Jan 2, 2025

maybe this what you all meant by being consistent (having proper rules?) lol sorry maybe we discussed this already in some other discussion or somewhere. Gonna go through discussions again.

Yes!

That + type checking seems the way to go. I think this can be a part of #170 / #180

@rin-st
Copy link
Member Author

rin-st commented Jan 3, 2025

So then we close this PR and allow only directly passing of strings? If people pass array we throw error?

Yes, closing this PR then. But add error a bit later in second issue, see below:

Created two issues

One for setting rules of passing arguments: #190
Second is for typecheck #191

That + type checking seems the way to go. I think this can be a part of #170 / #180

Umm I think we can add info about all related issues/fixes to #170 if we want all of them to be in one place

@rin-st rin-st closed this Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants