Skip to content
This repository was archived by the owner on Apr 1, 2020. It is now read-only.

Bugfix/Fix dynamic import for shell env #2640

Merged
merged 11 commits into from
Oct 27, 2018

Conversation

akinsho
Copy link
Member

@akinsho akinsho commented Oct 18, 2018

The shell env package has been silently failing meaning that fixes that allow oni to read in a user's env var have regressed. This happened when we upgraded webpack because with the new version of webpack, dynamic imports now return objects with the shape { default: DefaultFnExport, aVar: var }

Also upgraded the shellenv package as is since been upgraded.

@codecov
Copy link

codecov bot commented Oct 18, 2018

Codecov Report

Merging #2640 into master will increase coverage by 0.27%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2640      +/-   ##
=========================================
+ Coverage   45.43%   45.7%   +0.27%     
=========================================
  Files         361     361              
  Lines       14563   14603      +40     
  Branches     1912    1917       +5     
=========================================
+ Hits         6617    6675      +58     
+ Misses       7721    7700      -21     
- Partials      225     228       +3
Impacted Files Coverage Δ
...src/Services/Configuration/DefaultConfiguration.ts 87.5% <ø> (ø) ⬆️
browser/src/Plugins/Api/Process.ts 54.16% <25%> (-3.41%) ⬇️
...es/SyntaxHighlighting/SyntaxHighlightingReducer.ts 30.95% <0%> (-4.19%) ⬇️
.../Services/SyntaxHighlighting/SyntaxHighlighting.ts 22.91% <0%> (-1%) ⬇️
browser/src/neovim/NeovimWindowManager.ts 10.61% <0%> (-0.6%) ⬇️
...ices/SyntaxHighlighting/SyntaxHighlightingStore.ts 25.86% <0%> (-0.46%) ⬇️
browser/src/Editor/NeovimEditor/NeovimEditor.tsx 8.62% <0%> (ø) ⬆️
browser/src/Editor/BufferHighlights.ts 16.66% <0%> (ø) ⬆️
...es/SyntaxHighlighting/SyntaxHighlightReconciler.ts 79.59% <0%> (+0.04%) ⬆️
browser/src/neovim/NeovimTokenColorSynchronizer.ts 78.94% <0%> (+0.1%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ae1224...a506063. Read the comment docs.

@akinsho
Copy link
Member Author

akinsho commented Oct 20, 2018

Whilst the current change restores previous env reading functionality an issue which it raises (this was raised as a separate issue I currently can't find) is that if a user has a problematic script or command in their zsh or bashrc etc. then oni's initialisation will hang. As an example I previously used oh-my-zsh and had no issues sourcing my zshrc to read in my env vars, however since switching to zplug which adds a command to source some scripts in the rc file, booting oni now blocks.

This may have something to do with running the shell interactively or not, its not clear what the exact issue is, I think an interim solution is raising the issue with users that complex scripts might block the process and should have a guard around them (not sure what form this would take, would be specific to the users rc file) or perhaps forking shellenv or raising a PR/issue to have it not block in those cases

@akinsho
Copy link
Member Author

akinsho commented Oct 20, 2018

Following further investigation seems the issue only occurs as a result of very unusual rc file settings in my case a very obscure bug in zplug, a work around I've added in as this issue also occurred (at another point, though I cant find it 💢) is a config option called oni.userShell this allows a user to have oni derive the $PATH and other shell variables from a different shell if another is causing an issue. Tbh this is more of a workaround that allows oni to run but I think this issue is v. v. unlikely to hit users and if it does is usually I think the result of an offending line in the zshrc or bashrc which means pulling vars out of it doesnt work which shouldnt be the case for a normal functioning rc files (history may prove me wrong 😆 )

@akinsho akinsho requested a review from CrossR October 20, 2018 21:07
@akinsho akinsho changed the title Feature/Fix dynamic import for shell env Bugfix/Fix dynamic import for shell env Oct 20, 2018
Copy link
Member

@CrossR CrossR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gave it a test on Windows and env looks how I'd expect!

Looks good!

@akinsho akinsho merged commit 8bb8a9f into onivim:master Oct 27, 2018
@akinsho akinsho deleted the feature/fix-shell-env-import branch October 27, 2018 09:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants