-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 pasting unicode into terminal sometimes not working #492
Conversation
I think that this could also be classified as a bug in |
Signed-off-by: Sebastian Malton <smalton@mirantis.com>
Yes, makes sense. |
|
||
// Overwrite PATH on darwin | ||
if (process.env.NODE_ENV === "production" && process.platform === "darwin") { | ||
process.env["PATH"] = env.PATH | ||
} | ||
|
||
let key = null | ||
for(key in env) { | ||
if(!env.hasOwnProperty(key) || process.env[key]) continue // skip existing and prototype keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kke do you remember why did we have this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The loop will go through all of the JS prototype object's functions and what ever without that. This check will limit the loop to only go through the actual user defined attributes.
And the || process.env[key]
is to avoid overwriting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to explicitly overwrite LANG
here. Maybe it could be done similar way as PATH
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. But it could also have been done stupidly orginally :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this fiddle: https://jsfiddle.net/2v3pdbqL/
I think that I should have put ...env
first because JS overwrites the first's fields when joining.
Signed-off-by: Sebastian Malton <smalton@mirantis.com>
Signed-off-by: Sebastian Malton <smalton@mirantis.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* add LANG env with UTF-8 specified * fix precedence of union of env objects Signed-off-by: Sebastian Malton <smalton@mirantis.com> Co-authored-by: Sebastian Malton <smalton@mirantis.com>
shell-env
) which for some reason broke pasting utf-8 unicode into the terminal emulator.Fixes #351
Signed-off-by: Sebastian Malton smalton@mirantis.com