Expose process.env from sandbox renderer #12166
Conversation
|
@juturu in your pull request, we'd appreciate if you could provide
|
There was a problem hiding this comment.
Why not simply make process.env redirect to the browser process env? It is not like the renderer will have a different environment. If there are differences, it will probably be internal to chromium and not useful for electron apps.
If you simply copy process.env from the browser process at renderer initialization (eg process.env = remote.process.env), the implementation will be much simpler and in practice it will have the same effect.
Another option would be having process.env as an alias to remote.process.env, which would allow one to pick changes to the browser process environment (though how useful that would be I can't say for sure)
|
@codebytere updated the description. |
| if (property->IsSymbol()) { | ||
| return info.GetReturnValue().SetUndefined(); | ||
| } | ||
| #ifdef __POSIX__ |
There was a problem hiding this comment.
Should be #if defined(OS_POSIX).
|
@tarruda the reason i went with this approach was to bring parity with non sandbox renderer.
There will be cases where main process is changing process.env and reading from renderer will not work.
I'm not in favor of using remote as it is synchronous retrieval and can cause hangs in apps. Let me know if see any issues with this. |
This is how it currently works in non-sandboxed renderers, the |
|
Thanks @MarshallOfSound. If we don't have these scenarios then i can remove the setter/deleter and enumerator methods on |
As @MarshallOfSound explained, the environment is not linked. It is inherited when the process is spawned and then becomes independent. Retrieving the browser process environment at startup via remote would have the same practical effect.
We already use synchronous IPC during startup to read the preload script, one more call to retrieve the environment won't have a significant impact. If optimization is a concern, you can modify the RPC that returns the preload script to return
I'd rather simply use |
juturu
force-pushed
the
sandbox_expose_processenv
branch
2 times, most recently
from
08f332e
to
3092e4c
Compare
|
@tarruda @MarshallOfSound i made the change to JS. If i run through a quick-start app, i see process.env getting reflected with this change as expected. Also variables defined in main process can be retrieved in renderer process. But, the test i added seem to not get just the variable |
spec/api-browser-window-spec.js
Outdated
| assert.equal(test, 'foo') | ||
| done() | ||
| }) | ||
| process.env.main = 'foo' |
There was a problem hiding this comment.
Any issue you can spot in the test?
This is the issue: The tests are run in an electron renderer and you are assigning to the renderer environment. To assign to the browser process environment, use remote.process.env.main = 'foo'
|
Looks good, but I suggest to squash the commits and reduce cluttering of master branch |
juturu
force-pushed
the
sandbox_expose_processenv
branch
from
cf3713a
to
b7684a7
Compare
juturu
force-pushed
the
sandbox_expose_processenv
branch
from
b7684a7
to
ec083b6
Compare
In non sandbox renderer, apps can use process.env as a way to pass data from main process to renderer process.
For example:
In main process, set process.env.foo = 'bar'. This value can be retrieved in the renderer process using process.env.foo.
This PR provides this functionality in sandbox renderer process.