onivim / oni

Oni: Modern Modal Editing - powered by Neovim
https://www.onivim.io
MIT License
11.35k stars 299 forks source link

Bugfix/Fix dynamic import for shell env #2640

Closed akinsho closed 5 years ago

akinsho commented 5 years ago

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[bot] commented 5 years ago

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% <ø> (ø) :arrow_up:
browser/src/Plugins/Api/Process.ts 54.16% <25%> (-3.41%) :arrow_down:
...es/SyntaxHighlighting/SyntaxHighlightingReducer.ts 30.95% <0%> (-4.19%) :arrow_down:
.../Services/SyntaxHighlighting/SyntaxHighlighting.ts 22.91% <0%> (-1%) :arrow_down:
browser/src/neovim/NeovimWindowManager.ts 10.61% <0%> (-0.6%) :arrow_down:
...ices/SyntaxHighlighting/SyntaxHighlightingStore.ts 25.86% <0%> (-0.46%) :arrow_down:
browser/src/Editor/NeovimEditor/NeovimEditor.tsx 8.62% <0%> (ø) :arrow_up:
browser/src/Editor/BufferHighlights.ts 16.66% <0%> (ø) :arrow_up:
...es/SyntaxHighlighting/SyntaxHighlightReconciler.ts 79.59% <0%> (+0.04%) :arrow_up:
browser/src/neovim/NeovimTokenColorSynchronizer.ts 78.94% <0%> (+0.1%) :arrow_up:
... 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 commented 5 years ago

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 commented 5 years ago

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 😆 )