Skip to content
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

Spin fails to generate Bucklescript project even when npm is installed #67

Closed
johnridesabike opened this issue Mar 24, 2020 · 10 comments
Closed
Labels
bug Something isn't working

Comments

@johnridesabike
Copy link

Using Spin 0.5.1 on MacOS and running spin new bs-react spin-test

πŸ—οΈ  Creating a new bs-react in spin-test
Done!
[...config options here]
🎁  Installing packages. This might take a couple minutes.
😱  External command not available: yarn. Please install it and try again.

I don't have yarn installed. It would be nice if was smart enough to use either yarn or npm.

@tmattio tmattio added the enhancement New feature or request label Mar 25, 2020
@tmattio
Copy link
Owner

tmattio commented Mar 25, 2020

Hi @johnridesabike, thanks for reporting the issue!

Without adding extra complexity and detecting installed commands, a fix for this could be to use npm instead of yarn, since I think it comes by default when installing node, so it would be safer to assume that users have npm installed on their systems.

@tmattio tmattio added bug Something isn't working and removed enhancement New feature or request labels Mar 25, 2020
@tmattio tmattio changed the title 😱 External command not available: yarn. Please install it and try again. Spin fails to generate Bucklescript project even when npm is installed Mar 25, 2020
@johnridesabike
Copy link
Author

Would it make sense to make an option for it? I’m imagining something like:

Install dependencies with npm? [Y/n]

And if you select n it can say something like:

Setup complete. Before you begin development, you’ll need to install the dependencies with npm or yarn.

I imagine that anyone with yarn installed is familiar enough with it to know how to run it themselves. This approach is still flexible enough to let users use whichever package manager they want.

@tmattio
Copy link
Owner

tmattio commented Mar 25, 2020

@johnridesabike I'm a bit hesitant to add new prompts. I feel like Spin already asks too many questions when generating projects. If there is a satisfying solution to this that does not require adding a new prompt, I'd prefer to look into it.

In fact, thinking about your first suggestion, I think the cost of the added complexity of using one command or the other depending on the user's system will be justified if the alternative is to add another prompt.

@citizen428
Copy link
Collaborator

@tmattio While not quite as convenient, skipping the post-install command and outputting a message like "Please run yarn/npm install to install all dependencies" seems like an option that could at least be considered.

@tmattio
Copy link
Owner

tmattio commented Mar 26, 2020

@citizen428 Good idea! Actually, we could tweak the behavior of the post-install commands to make them optional. If the post-install command is optional, it will run only if the user has the command on its system, otherwise, it would just ignore it, or eventually write a non-error message. What do you think?

EDIT: Maybe that's what you meant by "skipping the post-install"?

@citizen428
Copy link
Collaborator

citizen428 commented Mar 26, 2020

Your proposed solution is slightly more sophisticated than what I had in mind, I wouldn't even have attempted to run the command, just unconditionally output the message.

I like your approach though, as we'd essentially just have to change the following pattern match:

        switch (
          Utils.Sys.exec("which", ~args=[|el.command|]) |> Lwt_main.run
        ) {
        | WEXITED(0) => ()
        | _ => raise(Errors.External_command_unavailable(el.command))
        };

Instead of raising Errors.External_command_unavailable we could just print an error like

${command} unvailable.
Please run ${command} ${args}" 

On second thought, there's a lot happening after that switch, which we may need to move into the first match clause.

@citizen428
Copy link
Collaborator

citizen428 commented Mar 26, 2020

@tmattio I quickly hacked something together, this is what it looks like:

Screen Shot 2020-03-26 at 23 24 58

@johnridesabike
Copy link
Author

For what it's worth, Create React App will automatically use either yarn or npm: https://github.com/facebook/create-react-app/blob/master/packages/create-react-app/createReactApp.js#L333

@citizen428
Copy link
Collaborator

Thanks @johnridesabike! But I do think it makes more sense for Spin to have consistent handling of all post-install commands rather than a special case for npm/yarn.

@tmattio
Copy link
Owner

tmattio commented May 13, 2020

A fix for this is implemented in #74. The bs-react template will use yarn if it is available, or npm if not.

We define this in the template like this:

(post_gen
  (actions 
    (run yarn install))
  (message "🎁  Installing packages. This might take a couple minutes.")
  (enabled_if (not (run which yarn))))

(post_gen
  (actions 
    (run npm install))
  (message "🎁  Installing packages. This might take a couple minutes.")
  (enabled_if (run which yarn)))

Closing this now since I want to merge the PR in the next days (and tidy up the issue board at the same time).

@tmattio tmattio closed this as completed May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants