joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Initialize path before checking Node installation #300

Closed joshwcomeau closed 5 years ago

joshwcomeau commented 5 years ago

Final fix for the electron-builder switch. Needed to reconfigure my initializePath method to return a promise, and then wait for it to complete before checking the Node version.

codecov[bot] commented 5 years ago

Codecov Report

Merging #300 into master will decrease coverage by 0.06%. The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
- Coverage   20.49%   20.42%   -0.07%     
==========================================
  Files         239      239              
  Lines        3733     3740       +7     
  Branches      385      387       +2     
==========================================
- Hits          765      764       -1     
- Misses       2690     2696       +6     
- Partials      278      280       +2
Impacted Files Coverage Δ
src/services/platform.service.js 44.82% <0%> (-13.51%) :arrow_down:
src/components/Initialization/Initialization.js 30.3% <0%> (-1.96%) :arrow_down:
src/services/shell.service.js 30% <0%> (ø) :arrow_up:
joshwcomeau commented 5 years ago

Whoops, my bad. Removed .docz directory

mathieudutour commented 5 years ago

So I dug into the source of fix-path and what it is doing is changing process.env.PATH and not window.process.env.PATH. That's why it's not working. We should just remove it

joshwcomeau commented 5 years ago

So I dug into the source of fix-path and what it is doing is changing process.env.PATH and not window.process.env.PATH. That's why it's not working. We should just remove it

Good catch. I forked fix-path and tried to get it to use window.process.env instead, but ran into some other issues, felt like too deep of a rabbit hole to go down right now.

joshwcomeau commented 5 years ago

Yet another morning spent debugging, when all of a sudden, this was what was happening:

screen shot 2018-10-28 at 9 46 14 am

After a bunch of trial and error, I was able to reproduce an error: window is not defined. I have since lost the ability to reproduce that error, but happily I remember what it was.

Looking at the code, it really doesn't make sense. The only new window. invocation is in initializePath, which is only called in a componentDIdMount method. Given that we arent server-side rendering or anything, window should always be defined.

It turns out, this has to do with async/await x__x. Our componentDidMount is an async function, and for some reason that means it gets invoked really early?

I converted it to a standard promise chain, and now it seems to work. I don't know why it doesn't work with async/await, but frankly I'm too tired to dig into it anymore.

joshwcomeau commented 5 years ago

Also, to recap: I tested this with my default installation, through NVM, and also with no Node installed (by commenting out the NVM stuff from the PATH in .bashrc). If anyone else can test this on Windows just to double-check that I didn't break anything, that'd be awesome. Looking forward to getting this release out.

joshwcomeau commented 5 years ago

Gonna merge this since it's been tested cross-platform, and we're still in beta releases anyway. @mathieudutour if you have any concerns, happy to address in a followup patch version :)