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

yarn package manager #1963

Conversation

LostKobrakai
Copy link
Contributor

Dynamically determine which node package manager will be used. It does use the faster yarn to install dependencies if installed.

@josevalim
Copy link
Member

Thank you @LostKobrakai but I would like to wait a bit more time. Yarn is one day old and we should wait for it to get a bit more mileage before integrating it into Phoenix. If you prefer to use yarn, you can always skip the dependency installation step and then run yarn afterwards.

@josevalim josevalim closed this Oct 12, 2016
@jeroenvisser101
Copy link

jeroenvisser101 commented Nov 2, 2016

@josevalim Yarn is a lot faster though, and since it's now already 22 days old... Phoenix wouldn't force you anything, but if you have yarn installed locally, it would use yarn instead of npm, sounds alright to me?

@nekath
Copy link

nekath commented Jan 14, 2017

Time passed and we see a lot of traffic around yarn. (more than 460k downloads of the package). I think we should reconsider adding integration with yarn.

@brandonmikeska
Copy link

Any news on yarn, especially if we only use it if we detect it, that way it doesn't affect anyone who isn't using it?

@chrismccord
Copy link
Member

No existing plans for support

@ryanwinchester
Copy link

ryanwinchester commented Apr 14, 2017

Guys. If you want to use yarn, just use yarn. ¯_(ツ)_/¯
That's what I do (and every other sane person).

Also, it's literally 5 keystrokes, so not a big deal if Phoenix doesn't want to support it yet. 😜

@brandonmikeska
Copy link

Yeah I don't think anyone is making a big deal out of it :), just seeing if there were plans down the road to take that 5 key strokes down to one

@Gazler Gazler mentioned this pull request Apr 21, 2017
@josevalim
Copy link
Member

@LostKobrakai would you be interested in rebasing this PR so we can finally merge it? You need to rebase because master uses webpack. :) Thank you.

@josevalim josevalim reopened this Mar 7, 2018
@LostKobrakai
Copy link
Contributor Author

I can certainly do that in the next few days.

@pragmaticivan
Copy link

Hey @LostKobrakai maybe it would require more checks for that feature.

npm nowadays also creates a package-lock.json for version consistence.

It would also require to check for yarn.lock to determine which one to use.

I think yarn still don't check package-lock.json content. yarnpkg/yarn#3614

@josevalim
Copy link
Member

@pragmaticivan this is when just creating a new project, so there won't be any lock.

@pragmaticivan
Copy link

@josevalim ahh my bad, makes sense, thanks!

Dynamically determine which node package manager will be used. It does use the faster yarn to install dependencies if installed.
@LostKobrakai LostKobrakai reopened this Mar 19, 2018
@@ -145,7 +145,7 @@ defmodule Mix.Tasks.Phx.New do
webpack_pending = install_webpack(install?, project)
Task.await(compile, :infinity)

if Project.webpack?(project) and !System.find_executable("npm") do
if Project.webpack?(project) and !elem(node_package_manager(), 0) do
Copy link
Member

@Gazler Gazler Mar 19, 2018

Choose a reason for hiding this comment

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

Could we avoid using elem here? The pattern matching approach used in install_webpack is much clearer.

Another option is to return the package manager command or nil from node_package_manager()

if Project.webpack?(project) and !node_package_manager() do

defp node_package_manager() do
  cond do
    System.find_executable("yarn") -> 
      "yarn"
    System.find_executable("npm") ->
      "npm install"
    true ->
      nil
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really like the explicit return of "npm install" for the command logged into the console if npm/yarn is not available, but maybe it's enough to let install_webpack default to that command.

Copy link
Member

@Gazler Gazler Mar 19, 2018

Choose a reason for hiding this comment

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

@LostKobrakai that's a good point. In that case, I'd go for:

{pkm_available?, _} = node_package_manager()
if Project.webpack?(project) and !pkm_available? do

@Fudoshiki
Copy link

I use yarn, but new npm@6 seems pretty fast

@OvermindDL1
Copy link
Contributor

Yeah I would absolutely opt for not installing another package manager since npm is 'already' one and is 'already' required by yarn anyway, while yarn adds nothing that I've seen (please correct me if it does otherwise! I'm quite curious) except some speed (and maybe not in comparison to npm anymore anyway) but even then deps are downloaded generally only once per project anyway.

@LostKobrakai
Copy link
Contributor Author

Can we please not discuss the usefulness of yarn here? The installer does just a few lines of checking if yarn is installed and if so uses that instead of npm, otherwise it'll still fallback to npm. Also this pr is here since over a year now. So if the phoenix team decides to merge it it's certainly not without giving it the proper thought.

@chrismccord
Copy link
Member

Thanks @LostKobrakai! I have decided not to move forward on this feature. It would be an implicit lookup and another thing to go wrong outside of the team's direct control. npm has had improvements as well since yarn's release, and users are free as always to use yarn on their new projects themselves. Thanks!

@zoedsoupe
Copy link

I understand it, however npm stills so dam slow! Any chance of going back on this decision? Yeah, I could start a new project and then remove npm-related files but thats not a really good user experience.

@ryanwinchester
Copy link

I understand it, however npm stills so dam slow! Any chance of going back on this decision? Yeah, I could start a new project and then remove npm-related files but thats not a really good user experience.

It's such a minimal amount of work to change it. (I even swap out all the webpack stuff because I don't even use webpack directly and that's not bad, either). If you are creating enough new phoenix projects that this significantly impacts your productivity, write a script that does what you want.

npm is generally installed alongside nodejs and is the default package manager. The team decided to default to the default, so please just let this die.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.