Closed joshwcomeau closed 6 years ago
AWolf81
commented
6 years ago @joshwcomeau I'm working on this. Nothing pushed yet.
How should this work?
I think we could check if node is installed with node -v and if it fails show an error message with details how to fix the problem.
I would add this check to refresh-projects.saga and bail early. Then we're not showing anything from the app as the app-loaded won't be true.
What do you think is it OK to implement it like this?
AWolf81
commented
6 years ago OK, I've pushed my WIP to branch node-avail-check.
It should work but not tested yet. I need to test it with the binary as during devServer I need node present.
I added the following:
checkIfNodeIsAvailable inside shell.serviceREADME.md into a default dialog.) I think this could be a separate issue as it would be great to have nicer looking custom dialogs.Todos:
joshwcomeau
commented
6 years ago Ah, so I think rather than create our own custom modal to support nested links, we should just auto-open a browser window when they click "Ok". Since yeah it's not really an optional thing, they can't continue using Guppy until they solve it.
I'm also a fan of using native platform options, since they tackle a lot of things for us (like keyboard controls, focus management, disabling scrolling, etc). So at some point we may need to create our own, but I'd like to defer that until there's something we really can't work around.
(there is also the fact that we could make it look nicer, but I think aesthetics are less important than familiarity, and I think it's good that we can use dialogs that the user is already very familiar with)
EDIT: For the copy, I think it could be something like:
It looks like Node.js isn't installed. Node is required to use Guppy.
When you click "OK", you'll be directed to instructions to download and install Node.
joshwcomeau
commented
6 years ago How should this work? I think we could check if node is installed with node -v and if it fails show an error message with details how to fix the problem.
I would add this check to refresh-projects.saga and bail early. Then we're not showing anything from the app as the app-loaded won't be true.
What do you think is it OK to implement it like this?
For the most part, that sounds great! It makes a lot more sense than what I was originally thinking :+1:
One small thing: I'm not sure if it should be as part of the refresh-projects saga. It feels like a separate concern.
Maybe we can just call the service right from within App.js? I'm not sure it fits in with any of our existing sagas, and I'm also not sure it deserves its own, since it's just a simple check.
joshwcomeau
commented
6 years ago (sorry for all the messages!)
One thing I just thought of, maybe for a future release. I wonder if we can install Node from within Guppy?
We might run into permissions issues, but it'd be cool to try and install NVM from within Guppy. It would require its own UI and saga and stuff, so I think we should tackle this later, but it's kind of a cool idea, right?
AWolf81
commented
6 years ago @joshwcomeau Thanks for your feedback.
I've changed it as you proposed (not pushed yet). I've added it to App.js and you're right it's not related to refresh-saga - I just wanted to stop rendering the app if Node is missing.
But I think it's also OK to render the app and show the error message to inform the user that Node is required. Now I have to add the same dialog to the Create-project-wizard to avoid the stuck installation process. Is it OK to show a different message there with-out a redirect to the installation guide?
Yes, a Saga seems a bit to much for the simple check. That was my first thought but haven't implemented it.
What do you think should we attach a node property to Mixpanel event? So we know how often this happens. e.g. logger.logEvent('load-application', {'node_version': 'missing' | 'v8.x.y'});
Then we also know which node versions are used & how often node is missing.
I'll open a PR for this in a few Minutes.
If the user downloads Guppy without having Node.js installed, Guppy will open without issue.
In fact, nothing will go wrong until they try creating a new project. At that point, a console error is logged, but nothing is shown in the UI :/ this screen will hang forever:
We should perform a check on mount that sees if it can find a valid Node.js installation. If it can't, it should throw a warning dialog, telling the user to install node, and linking them to our installation instructions.
Note that in the future, we hope that the user won't have to download Node separately (see #44). Unfortunately, this task seems non-trivial, so we're deferring it for now, and we need a stop-gap measure.