fix: ensure PATH is always set #93
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eduardoboucas
previously approved these changes
Aug 9, 2022
danez
changed the title
danez
changed the title
danez
force-pushed
the
path
branch
4 times, most recently
from
7da11ee
to
57b6bfd
Compare
danez
changed the title
| @@ -199,6 +200,9 @@ class DenoBridge { | |||
| env.DENO_DIR = this.denoDir | |||
| } | |||
|
|
|||
| // Ensure PATH is always set as otherwise we are not able to find the global deno binary | |||
| env[pathKey()] = inputEnv[pathKey({ env: inputEnv })] || process.env[pathKey()] | |||
There was a problem hiding this comment.
Doesn't pathKey always need the environment variables passed in?
If I look at this correctly, and if the variable happens to be PATH on Windows, you're then setting it as Path in the final env object?
Or am I misunderstanding? 🤔
There was a problem hiding this comment.
It sets path to whatever the PATH casing is in process.env.
So if someone passes in PATH, it will be converted to Path on windows, or whatever the current casing is.
So from the outside of the bridge you do not need to care about windows or not.
eduardoboucas
approved these changes
Aug 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #82 broke the detection of global installed binaries. The detection of the binary uses all environment variables and finds the global binary. When we then run the edge function with
deno ...deno is not found because PATH is not there anymore.I don't like this solution, but I like the alternative even less. The only alternative i could come up is using
node-whichpackage to find the absolute path to the global binary. But not sure if we really want to add that overhead?What do you think? This means that edge functions in dev will have PATH available.