If I understand correctly we add the variables to the current process and then we start the server cloning all the variables.

if so I'd suggest to just pass the variables to the server without this back and forth.

Yes that is a better idea.

Also I noticed that we have a split between system_vars (I think are the variables set with the tuono command [do we need to support them?]) and the defined in the .env files. Do we need such split?

Yes we should, because these are also other system variables that may be present. They should take priority.

For the record, I'd personally split the env_manager and watch reload PRs. Too broad the scopes

I see how that might be more aligned with atomic PRs, but then we are adding a struct with no usages. The actual bit that handles hot reload is only 10 lines or so, and whilst ordinarily I think they should be split, I think in this case it is better together since the reviewer needs to have the EnvVarManager in mind.

Yes we should, because these are also other system variables that may be present. They should take priority.

My point is that we should prevent the users to start tuono with some ENV variable declared from the command line. But I see your point. Better to have both.

I see how that might be more aligned with atomic PRs, but then we are adding a struct with no usages.

Good for me keeping everything here. I think this is not the meaning of atomic PR tho. For my experience doing atomic PR means scoping even smaller PR for every change (i.e. create struct, read var, reload var, pass var to process, etc).

This because at each review we know exactly what part of the feature you are addressing and it is easier reviewing it and challenging the implementation. 250 new code rows are very though to challenge since there are too moving parts.

Knowing the context of a change should come from a meaningful PR description rather than the whole code (we expect that the previous code is correct since already reviewed).

No worries here. Let's keep in mind that Atomic PR is meant for better challenging the PRs to deliver higher quality

This because at each review we know exactly what part of the feature you are addressing and it is easier reviewing it and challenging the implementation. 250 new code rows are very though to challenge since there are too moving parts.

Knowing the context of a change should come from a meaningful PR description rather than the whole code (we expect that the previous code is correct since already reviewed).

No worries here. Let's keep in mind that Atomic PR is meant for better challenging the PRs to deliver higher quality

Thanks for the feedback, that has made it much clearer. In future I will try to do this.

Alright. I'd just change the way we pass the data from the CLI to the server process (avoiding having the env cloned on both processes).

Ok. Here's my plan:

• We load all system env into a Hashmap (env_vars or whatever it's called) during App::new
• We load in all env values during App::new (even if that's just calling reload_variables) like you suggested. I agree - I think doing that is better for the code base overall
• We refresh this like we are doing
• We pass this hashmap as env like you suggested

My concerns with this approach:

• Loading all env is bad for performance - I don't know how to solve this since we can't differentiate between what is passed before the command and what is always present.

I've resolved all of code review, because I'd like to discuss this with you more, so that we can figure out the best implementation together.

I'm really open to suggestions or big changes - please let me know :)

Ok. Here's my plan:

Agree. (Do we need two separate variables for storing system and user defined variables?)

Loading all env is bad for performance

Why you think that? What performance you think we are dropping?

The CLI is just a fancy interface to deal with the tuono project. IMHO, we should always prefer a better codebase rather a top performance process (just for the CLI).

tuono_lib is the server, so in this crate we have to strictly prefer improving performance rather than a more readable/understandable code.

Why you think that? What performance you think we are dropping?

We would have to load the name and value every variable present on the system, even those completely unrelated to tuono. (Run printenv on linux and you can see these). This is because I don't think there is a way to tell the difference betwen variables set before the command (eg MY_VAR=hello tuono dev), and the rest of system environment variables.

Maybe this is just not really an issue, but it doesn't feel like the right way to do this. In fact it looks like this is what nodejs does anyway (and I think vite just uses node process.env), so maybe let's just do it? WDYT

The CLI is just a fancy interface to deal with the tuono project. IMHO, we should always prefer a better codebase rather a top performance process (just for the CLI).

Agree.

This is because I don't think there is a way to tell the difference betwen variables set before the command (eg MY_VAR=hello tuono dev), and the rest of system environment variables.

Do we have to do this split? What is the benefit of knowing where they came from?

Maybe this is just not really an issue, but it doesn't feel like the right way to do this. In fact it looks like this is what nodejs does anyway (and I think vite just uses node process.env), so maybe let's just do it? WDYT

I haven't understood what you are referring to... what should we do?

The user's OS also has environment variables (eg PATH), and there are many which aren't relevant to tuono. When you set a variable like this MY_VAR=hello tuono dev, you have no way to distinguish it from all the other environment variables that are set on the OS. As consequence, you must load all of them into the CLI, and pass them to the server.

I was worried because I thought loading all of them was bad, however I think that is just the way they work. So I think it is the right decision to do that (#583 (comment)), since every process should have access to all environment variables.

I hope that made a bit more sense (my worries were pretty unfounded I think, so that might be what confused you).

I will start implementing this today.

I think that is not our problem. We should just ignore them.

tuono crate has full access to them as well as tuono_lib. What we should do is translating the .env defined variables to tuono_lib. Since they are system defined, there is nothing to do with them. They will be available out of the box on any process. Isn't it?

Agree. The only reason we have to interact with them to start with is so that they take precedence over ones from the file. We do still have to collect them because if we do .envs(var_hashmap), it will give the sub process only the vars from that hashmap, and not the system ones. We can do .envs(var_hashmap) if we put the system ones into the hashmap, which is what we will do.

So the plan can stay the same:

• We load all system env into a Hashmap (env_vars or whatever it's called) during App::new
• We load in all env values during App::new (even if that's just calling reload_variables) like you suggested. I agree - I think doing that is better for the code base overall
• We refresh this like we are doing
• We pass this hashmap as env like you suggested

Sorry for so much back and forth, but I think it's better to have it here than split through loads of code review sections. Hopefully review can be smoother now :)

We do still have to collect them because if we do .envs(var_hashmap), it will give the sub process only the vars from that hashmap, and not the system ones

Are you sure about that? I think every process has access to all the system env variables

Yes you're right, I'm sorry about all that - I was wrong. I read the docs and it looks like .env() adds to or updates them, however this takes precedence over system. I've changed the plan to prevent this and stay consistent with vite.

Updated plan then (changes are in bold italic):

• We load all names of keys in system env into a Hashmap (env_vars or whatever it's called) during App::new
• We load in only env vales that do not exist in system during App::new (even if that's just calling reload_variables) like you suggested.
• We refresh this like we are doing
• We pass this hashmap as env

We are on the same page! ⚡

I think we should go back to using std::env more. I feel I am reinventing the wheel a bit here, and the code is getting incredibly complex.

I think we should go back to using std::env more.

What do you mean?

I feel I am reinventing the wheel a bit here,

What are you reinventing?

The issue is there's not a way to pass in environment variables here: https://github.com/tuono-labs/tuono/blob/19207d7d907fc4598508a5f06f9cfb3f7c29ffb6/crates/tuono/src/watch.rs#L42C1-L62C2.

We have to load into OS env at some point - if this is scoped to process, we should move all env code out of the CLI, and into tuono_lib. We should then change the build and dev (ran by cargo run called in the snippet I linked) to update and include these variables, by moving EnvManager there. WDYT?

Whilst I appreciate there have been loads of comments, I think EnvManager is still the right way.

Yeah I think the EnvManager is still the right choice.

Consider doing a poc with your proposal!