Add Yarn support in new apps using --yarn option #26836

Liceth · 2016-10-20T00:58:40Z

This add a package.json and the settings needed to get npm nodules integrated in new apps when the --yarn option is used (e.g rails new blog --yarn).

The next changes should be done to finish this:

Add package.json in new apps when --yarn option is passed
Run yarn install after bundle install in new apps
Add rails-ujs dependency to package.json
Add node_modules folder to asset paths
Fix existing broken tests
Add new tests
Changelog

rails-bot · 2016-10-20T00:58:42Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

guilleiguaran · 2016-10-20T01:17:08Z

railties/lib/rails/generators/app_base.rb

@@ -404,6 +412,49 @@ def run_bundle
        bundle_command("install") if bundle_install?
      end

+      def npm_path
+        commands = ["npm"]


I think we can add also to "yarn" command here (https://code.facebook.com/posts/1840075619545360)

guilleiguaran · 2016-10-20T01:17:58Z

railties/lib/rails/generators/app_base.rb

-          gems << GemfileEntry.version("#{options[:javascript]}-rails", nil,
-                                       "Use #{options[:javascript]} as the JavaScript library")
+
+          packagejson_exist = File.exist?("package.json")


We are using this line code in multiple parts of code so I think we can extract it to an package_json_exist? function

guilleiguaran · 2016-10-20T01:18:16Z

railties/lib/rails/generators/app_base.rb

@@ -33,6 +33,9 @@ def self.add_shared_options_for(name)
        class_option :javascript,         type: :string, aliases: "-j", default: "jquery",
                                          desc: "Preconfigure for selected JavaScript library"

+        class_option :npm,                type: :boolean, default: false,
+                                          desc: "Preconfigure for NPM for assets management"
+


"Preconfigure NPM for assets management"

guilleiguaran · 2016-10-20T01:20:04Z

railties/lib/rails/generators/rails/app/templates/package.json

+  "dependencies": {
+    "jquery": "~3.1.0",
+    "jquery-ujs": "~1.2.2"
+  }


I think we will be able to remove jquery from here and change jquery-ujs to rails-ujs after of having #25208 finished

guilleiguaran · 2016-10-20T01:21:26Z

railties/lib/rails/generators/app_base.rb

+
+        unless npm_path
+          puts %{Warning: npm option passed but npm executable was 
+            not detected in the system. Download Node.js in https://nodejs.org/en/download/}


Let's move this warning message to https://github.com/rails/rails/pull/26836/files#diff-248322211d56bb3b2ab59ac68a2839f5R212, right now it's being displayed everytime the npm_path function is called if command is not found

rafaelfranca · 2016-10-20T01:36:51Z

Can we change npm with yarn? I think only the npm install command that
needs to change
On Wed, 19 Oct 2016 at 21:58 Ruby on Rails Bot notifications@github.com
wrote:

Thanks for the pull request, and welcome! The Rails team is excited to
review your changes, and you should hear from @eileencodes
https://github.com/eileencodes (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra
commits. This ensures that the reviewer can see what has changed since they
last reviewed the code. Due to the way GitHub handles out-of-date commits,
this should also make it reasonably obvious what issues have or haven't
been addressed. Large or tricky changes may require several passes of
review and changes.

This repository is being automatically checked for code quality issues
using Code Climate https://codeclimate.com. You can see results for
this analysis in the PR status below. Newly introduced issues should be
fixed before a Pull Request is considered ready to review.

Please see the contribution instructions
http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html
for more information.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#26836 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAC66CqJBNFf1G3_6OUb7rSrr1YlfQVeks5q1rzGgaJpZM4KbnZu
.

Liceth · 2016-10-20T01:57:25Z

yes, we can add "yarn" as first option to supported commands list and it will be used instead of npm if installed

rafaelfranca · 2016-10-20T02:11:56Z

I'd not even bother to support npm, just jump straight to yarn.

On Wed, 19 Oct 2016 at 22:57 Liceth Ovalles Rodriguez <
notifications@github.com> wrote:

yes, we can add "yarn" as first option to supported commands list and it
will be used instead of npm if installed

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#26836 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAC66LVJhBn3WNYgCrQQT1xk7LRggIDcks5q1sqJgaJpZM4KbnZu
.

Liceth · 2016-10-22T22:50:01Z

@eileencodes @rafaelfranca @guilleiguaran This is ready for review and failures in TravisCI aren't related to my changes (mysql2 tests are failing since yesterday).

guilleiguaran · 2016-10-23T01:09:51Z

@Liceth I've fixed the mysql2 tests in f13ec72 and updated your branch

guilleiguaran · 2016-10-24T13:51:53Z

railties/lib/rails/generators/rails/app/app_generator.rb

@@ -205,6 +209,9 @@ def create_root_files
        build(:readme)
        build(:rakefile)
        build(:configru)
+        if options[:npm]
+          build(:packagejson)
+        end


This if can be done in a single line:

build(:packagejson) if options[:npm]

Thanks for the observation @guilleiguaran I already made the changes

bouk · 2016-10-25T14:38:21Z

Please note that yarn is not a total drop-in replacement for npm, we can't try one or the other. A project that uses yarn gets a yarn.lock file (analogous to Gemfile.lock). NPM doesn't create a file like that and doesn't interpret it.

I fully support not even bothering with NPM and just using yarn, it's a lot more robust in my experience. Requiring people to install it separately might take away from the plug-and-play nature of Rails.

Liceth · 2016-10-25T22:19:15Z

I'm fine wherever you decide about yarn/npm, only let me know the final decision on it.

connorshea · 2016-10-26T23:58:26Z

Don't you install Yarn using npm? I 100% support using Yarn as the package manager, but it won't avoid npm entirely.

connorshea · 2016-10-26T23:59:20Z

railties/lib/rails/generators/app_base.rb

@@ -33,6 +33,9 @@ def self.add_shared_options_for(name)
        class_option :javascript,         type: :string, aliases: "-j", default: "jquery",
                                          desc: "Preconfigure for selected JavaScript library"

+        class_option :npm,                type: :boolean, default: false,
+                                          desc: "Preconfigure for NPM assets management"


npm should be lowercase in all situations as far as I know.

connorshea · 2016-10-27T00:00:22Z

railties/lib/rails/generators/app_base.rb

+            npm_command("install")
+          else
+            say_status :warning, "npm option passed but npm executable was not detected in the system.", :yellow
+            say_status :warning, "Download Node.js in https://nodejs.org/en/download/.", :yellow


Should be "Download Node.js at https://nodejs.org/en/download/"

Hi @connorshea Thanks for you review

connorshea · 2016-10-27T00:09:06Z

railties/lib/rails/generators/rails/app/templates/config/initializers/assets.rb.tt

@@ -5,6 +5,9 @@ Rails.application.config.assets.version = '1.0'

 # Add additional assets to the asset load path
 # Rails.application.config.assets.paths << Emoji.images_path
+<%- if options[:npm] -%>
+Rails.application.config.assets.paths << Rails.root.join('node_modules')


Should this have a comment explaining what it does?

FND · 2016-12-02T16:20:54Z

@dhh's approach seems reasonable - however, it might still be advisable to choose a compiler-agnostic name for the CLI option, to prevent API changes when the underlying infrastructure changes? (For straightforward webpack configurations, Rollup can be used as a drop-in replacement.)

dhh · 2016-12-02T16:24:04Z

Don't think the CLI options matter that much, since even if we did go with a generic solution, we can still support that. So --webpack would trigger the Webpack variant of that. Then --other-pipe could trigger that. Or we could extend with --pipeline webpack. This CLI name is only relevant for the start of a new project anyway. So it's not like there'd be backwards incompatibility issues going forward.

But yeah, I think all this is up in the air. Let's get a real PR going, then it'll be easier to talk about specifics.

youngbrioche · 2016-12-02T16:25:44Z

+1 for --pipeline webpack.

kayakyakr · 2016-12-02T16:40:02Z

This triggered a long conversation in our Slack about the same and even with just 2 of us mostly talking, we came up with 4 different opinions on how it should be done... In general, I think we'd favor a combination of @moonglum and @dhh's approaches.

Dunno if anyone's had experience with https://github.com/thoughtbot/ember-cli-rails, but they've mostly hit the sweet spot as far as I'm concerned.

dhh · 2016-12-02T16:49:02Z

@kayakyakr That's always been a big part of the value with Rails. There's an infinite number of ways things COULD be done, but there's an immense benefit to simply picking a path of running with it. That's the core promise behind Convention over Configuration.

FND · 2016-12-02T17:00:45Z

FWIW, without wanting to belabor this point, just a quick clarification: The benefits of Convention over Configuration are of course undisputed - the reason this issue seemed worth raising is because the JavaScript precompiler shouldn't surface to application developers, i.e. the respective source code should be unaffected by whatever is decided here.

kayakyakr · 2016-12-02T17:08:48Z

@FND that's not going to be the case if the implementation is to run sprockets and webpack side-by-side, though. Each pipeline wants its own assets path. Only taking the path of full pipeline replacement gets you the ability to be precompiler-agnostic.

moonglum · 2016-12-02T17:16:58Z

In general, I think we'd favor a combination of @moonglum and @dhh's approaches.

I think that makes sense. If the ability to read manifest files as described above would be built-into Rails, then all the --webpack flag would need to do is create a webpack config that creates the according manifest files. Adding a --whatever-other-solution-you-need flag would then be simple.

This would offer the easy out of the box experience that @dhh described, but would still be flexible to use with other external asset pipelines.

NekR · 2016-12-04T03:13:31Z

I know that web pack technically CAN manage those things, but I don't see a big benefit to doing so.

It might be useful if someone would, say, use a ServiceWorker webpack plugin to generate it for static assets. I know there is ServiceWorker plugin for rails' itself, but still, that might be possible.

skull-squadron · 2016-12-04T06:19:37Z

Looks good. Previously there was webpack-rails (more resources: Migrating from Rails’ asset pipeline to Node’s webpack, react-webpack-rails-tutorial), which might serve as an example to build upon configuration / conventions and integration behaviors.

akabekobeko · 2016-12-05T07:28:00Z

webpack is popular and multi-functional. However, it is mere module and it is not standard as Node and npm. As with yarn, these are optional as npm and npm-scripts as standard.

If jQuery dependence is to be stopped, it is better to be cautious about non-standard product dependence.

If future yarn and webpack are obsolete (...It may be impossible), npm and npm-scripts will remain.

justin808 · 2016-12-05T08:49:06Z

@steakknife Thanks for the shout-out for the react-webpack-rails-tutorial. Its sibling package is react_on_rails.

justin808 · 2016-12-05T08:58:11Z

@dhh wrote:

Lucas, that's an interesting idea. I'd like to start with Webpack first and see how things go. For starters, we're only going to use Webpack for app-style JS. Not images, css, fonts, etc.

A huge advantage of Webpack is CSS-Modules. If you like tidy, you'll love CSS-Modules. See Sass, CSS Modules, and Twitter Bootstrap Integration.

Here's an example of it:

josepjaume · 2016-12-05T09:42:43Z

@dhh in my experience, the major pain points I've found integrating any sort of pre-compiling using Webpack and feeding it to Sprockets are asset fingerprinting. The way Sprockets works right now, it's nearly impossible to create, let's say, a react component that references a particular asset (an image, for example), without getting React to read Sprocket's manifest file, or Sprockets read Webpack's manifest file.

There's two approaches I believe might work:

The first one, just let Webpack handle asset fingerprinting, but you already said that's not desirable.
Another other option would be to let Sprockets fingerprint all the assets and rewrite the actual assets just at the end of the build process, instead of relying on the original code to write the right asset locations - something like what the Phoenix framework does with brunch. That'd would also decouple the contract between webpack (or any other build tool) and sprockets, allowing us to integrate with virtually any other build tool.

Maybe the guys on https://github.com/shakacode/react_on_rails/ (specially @justin808) could chime in on the matter, they seem to have given it a lot of thought.

justin808 · 2016-12-06T00:27:07Z

@josepjaume We have the problem solved in React on Rails, such that "it just works". The solution involves:

Allow webpack to fingerprint assets.
Publish the assets using the Rails asset pipeline, which creates double fingerprints.
Create symlinks at deployment, using the manifest file, so that we have single-fingerprinted symlink names that point to the double fingerprinted files.

This way, the correct file names are available on deployment.

Any questions?

dhh · 2016-12-07T18:17:26Z

For those interested, I've published the first work on --webpack here: #27288.

Re: cross-reading of manifests, @josepjaume, I think that's actually how we should make this work. Make it possible for each side to read the others manifest and construct what they need from there. The current strategy is to use Webpack exclusively for app-like JS, and nothing else. Asset pipeline for everything else (JS sprinkles, images, fonts, css, etc).

iantheparker · 2017-02-28T21:56:05Z

@josepjaume this is also how the bower-rails gem resolved relative asset paths.

Another other option would be to let Sprockets fingerprint all the assets and rewrite the actual assets just at the end of the build process, instead of relying on the original code to write the right asset locations

Has this option gotten any love recently?

cvkmohan · 2017-07-04T08:18:05Z

My Rails Version is 5.1.2 and Ruby 2.4 - I am using Yarn

yarn add font-awesome

Now, in application.css.scss I imported the font-awesome

@import font-awesome/scss/font-awesome;

Now, when I refer to fa-icon in the view, I am getting 404 Not found error for the font file in development. Any idea what I am doing wrong?

Schwad · 2017-07-25T10:56:35Z

Hey all, really interested in what's going on with these changes and updating from Rails 5.0 to Rails 5.1. Quick (hopefully not too stupid) question; does this only apply to new Rails Apps with rails new blog --yarn, or are there any out of the box options included for updating existing Rails 5.1 apps with this?

Thanks so much for your work on this feature!!! 🎉

aried3r · 2017-08-28T16:06:26Z

@Schwad, there is a rails yarn:install task, but it doesn't really do everything that happens when a new Rails app is created, notably editing the .gitignore file, adding node_modules to the assets path etc.

You can find a few things if you search for skip_yarn in the codebase.

I'm not sure if this could be changed or is an intentional omission, but I couldn't find an official guide for adding yarn to existing Rails projects upgrading to 5.1.

aried3r · 2017-08-29T15:19:46Z

@Schwad, also, if you run the rails app:update task, Rails will offer you to add all necessary changes except for the .gitignore file I think.

skull-squadron · 2017-08-30T14:42:56Z

Just came by to mass unsubscribe.
(start bitching)
I don't use Rails for new development anymore, but yarn and broccoli awesome. Yarn is face-meltingly fast and a nearly drop-in replacement for slow, unlocked npm. Webpack proved to be brittle, inflexible and slow, despite having a loyal, zealous herd. Hobby projects are free to use anything, but development for maintainable production has many different needs.
(end bitching)

amrutjadhav · 2018-02-19T03:32:32Z

@krushi123 You need to overwrite the $fa-font-path variable which is used by font-awesome to point to font directory. Just add these line to your application.css.scss:
$fa-font-path: "font-awesome/fonts/";
before
@import font-awesome/scss/font-awesome;

rails-bot assigned eileencodes Oct 20, 2016

guilleiguaran self-assigned this Oct 20, 2016

guilleiguaran changed the title ~~Add Npm support in new apps using option~~ [WIP] Add Npm support in new apps using option Oct 20, 2016

guilleiguaran changed the title ~~[WIP] Add Npm support in new apps using option~~ [WIP] Add npm support in new apps using --npm option Oct 20, 2016

guilleiguaran reviewed Oct 20, 2016

View reviewed changes

eileencodes added railties asset pipeline labels Oct 20, 2016

guilleiguaran added this to the 5.1.0 milestone Oct 20, 2016

Liceth force-pushed the npm branch from f29fbc5 to 821d4da Compare October 22, 2016 22:25

Liceth changed the title ~~[WIP] Add npm support in new apps using --npm option~~ Add npm support in new apps using --npm option Oct 22, 2016

guilleiguaran reviewed Oct 24, 2016

View reviewed changes

eileencodes removed their assignment Oct 25, 2016

guilleiguaran assigned rafaelfranca Oct 25, 2016

connorshea reviewed Oct 26, 2016

View reviewed changes

connorshea reviewed Oct 27, 2016

View reviewed changes

Liceth force-pushed the npm branch from a88eb70 to 54c75e1 Compare October 27, 2016 21:23

jamby mentioned this pull request Dec 6, 2016

Browserify/babelify transforms not working browserify-rails/browserify-rails#181

Closed

mtho11 mentioned this pull request Dec 13, 2016

[WIP] Angular 2 Migration Preparation(Draft) ManageIQ/manageiq#13131

Closed

16 tasks

toastercup mentioned this pull request Dec 14, 2016

Rails 5.1 Upgrade cortex-cms/cortex#405

Closed

This was referenced Jan 12, 2017

Empty Issue ManageIQ/manageiq-ui-classic#147

Closed

[WIP] Angular 2 Migration Preparation(Draft) ManageIQ/manageiq-ui-classic#148

Closed

guilleiguaran mentioned this pull request Jun 6, 2017

Loosen up yarn and support npm rails/webpacker#475

Closed

nathanbirrell mentioned this pull request Jul 4, 2017

**WIP** F68 spot view nathanbirrell/poseidon#19

Merged

Add Yarn support in new apps using --yarn option #26836

Add Yarn support in new apps using --yarn option #26836

Conversation

Liceth commented Oct 20, 2016 • edited by guilleiguaran Loading

rails-bot commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca commented Oct 20, 2016

Liceth commented Oct 20, 2016

rafaelfranca commented Oct 20, 2016

Liceth commented Oct 22, 2016 • edited Loading

guilleiguaran commented Oct 23, 2016

guilleiguaran Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bouk commented Oct 25, 2016

Liceth commented Oct 25, 2016 • edited by guilleiguaran Loading

connorshea commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FND commented Dec 2, 2016 • edited Loading

dhh commented Dec 2, 2016

youngbrioche commented Dec 2, 2016

kayakyakr commented Dec 2, 2016

dhh commented Dec 2, 2016

FND commented Dec 2, 2016

kayakyakr commented Dec 2, 2016

moonglum commented Dec 2, 2016

NekR commented Dec 4, 2016

skull-squadron commented Dec 4, 2016

akabekobeko commented Dec 5, 2016 • edited Loading

justin808 commented Dec 5, 2016 • edited Loading

justin808 commented Dec 5, 2016

josepjaume commented Dec 5, 2016 • edited Loading

justin808 commented Dec 6, 2016

dhh commented Dec 7, 2016

iantheparker commented Feb 28, 2017 • edited Loading

cvkmohan commented Jul 4, 2017 • edited Loading

Schwad commented Jul 25, 2017

aried3r commented Aug 28, 2017

aried3r commented Aug 29, 2017

skull-squadron commented Aug 30, 2017 • edited Loading

amrutjadhav commented Feb 19, 2018

Liceth commented Oct 20, 2016 •

edited by guilleiguaran

Loading

Liceth commented Oct 22, 2016 •

edited

Loading

guilleiguaran Oct 24, 2016 •

edited

Loading

Liceth commented Oct 25, 2016 •

edited by guilleiguaran

Loading

FND commented Dec 2, 2016 •

edited

Loading

akabekobeko commented Dec 5, 2016 •

edited

Loading

justin808 commented Dec 5, 2016 •

edited

Loading

josepjaume commented Dec 5, 2016 •

edited

Loading

iantheparker commented Feb 28, 2017 •

edited

Loading

cvkmohan commented Jul 4, 2017 •

edited

Loading

skull-squadron commented Aug 30, 2017 •

edited

Loading