spawnteract icon indicating copy to clipboard operation
spawnteract copied to clipboard

deep merge spawnOptions.env with kernelSpec.env

Open haraldschilly opened this issue 6 years ago • 2 comments

This ticket is about refining the behavior for setting the environment of a newly started kernel. Right now, as it can be seen in this code reference the various options are shallow-merged. I.e. if there is a kernel-configured environment and a custom one passed in via the spawnOptions object, spawnOptions.env overwrites everything.

My idea about this is -- at least optionally, such that existing behavior isn't broken -- to merge these env configurations before they're passed to execa, i.e. a "deeper merge". In particular, this would allow to overwrite a single environment variable from the kernel config file, while the other ones are kept as they are. I'm for example thinking about a case, where variables like LD_LIBRARY_PATH and PATH are configured in the kernel, but I want to tweak PATH via spawnOptions and keep LD_LIBRARY_PATH as it is.

Since at least I find this confusing, I'm just clarifying this by an example:

> env = { PATH : 1, VAR : 2}
{ PATH: 1, VAR: 2 }
> spawnOptions = {env : { PATH : 3}}
{ env: { PATH: 3 } }
> Object.assign({} , {env : env} , spawnOptions)
{ env: { PATH: 3 } }                  // VAR:2 gone!

merging env one level deeper preserves VAR

> Object.assign({}, env, spawnOptions.env)
{ PATH: 3, VAR: 2 }

haraldschilly avatar Feb 08 '19 20:02 haraldschilly

Interesting points. I think your idea makes sense if you wanted to make a PR.

I am glad you are bringing up how this works as I wondered about this when we switched to execa. At the time, I opened #34 because I was unclear on how execa handles this and if it would work for us.

See also: #24 #14

BenRussert avatar Feb 09 '19 15:02 BenRussert

@haraldschilly If you're interested in opening a PR to make this modification, you'll need to do so in the nteract monorepo. We're actually deprecating this package and moving the logic here to the fs-kernels package. The code has been moved to this file.

captainsafia avatar Feb 09 '19 20:02 captainsafia