-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Add Yarn support in new apps using --yarn option #26836
Conversation
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. |
@@ -404,6 +412,49 @@ def run_bundle | |||
bundle_command("install") if bundle_install? | |||
end | |||
|
|||
def npm_path | |||
commands = ["npm"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add also to "yarn" command here (https://code.facebook.com/posts/1840075619545360)
gems << GemfileEntry.version("#{options[:javascript]}-rails", nil, | ||
"Use #{options[:javascript]} as the JavaScript library") | ||
|
||
packagejson_exist = File.exist?("package.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using this line code in multiple parts of code so I think we can extract it to an package_json_exist?
function
@@ -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" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Preconfigure NPM for assets management"
"dependencies": { | ||
"jquery": "~3.1.0", | ||
"jquery-ujs": "~1.2.2" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will be able to remove jquery from here and change jquery-ujs
to rails-ujs
after of having #25208 finished
|
||
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/} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Can we change npm with yarn? I think only the npm install command that
|
yes, we can add "yarn" as first option to supported commands list and it will be used instead of npm if installed |
I'd not even bother to support npm, just jump straight to yarn. On Wed, 19 Oct 2016 at 22:57 Liceth Ovalles Rodriguez < yes, we can add "yarn" as first option to supported commands list and it — Reply to this email directly, view it on GitHub |
@eileencodes @rafaelfranca @guilleiguaran This is ready for review and failures in TravisCI aren't related to my changes (mysql2 tests are failing since yesterday). |
@@ -205,6 +209,9 @@ def create_root_files | |||
build(:readme) | |||
build(:rakefile) | |||
build(:configru) | |||
if options[:npm] | |||
build(:packagejson) | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if can be done in a single line:
build(:packagejson) if options[:npm]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the observation @guilleiguaran I already made the changes
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. |
I'm fine wherever you decide about yarn/npm, only let me know the final decision on it. |
Don't you install Yarn using npm? I 100% support using Yarn as the package manager, but it won't avoid npm entirely. |
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm should be lowercase in all situations as far as I know.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "Download Node.js at https://nodejs.org/en/download/"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @connorshea Thanks for you review
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have a comment explaining what it does?
@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.) |
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. |
+1 for |
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. |
@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. |
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. |
@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. |
I think that makes sense. If the ability to read manifest files as described above would be built-into Rails, then all the 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. |
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. |
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. |
If jQuery dependence is to be stopped, it is better to be cautious about non-standard product dependence. If future |
@steakknife Thanks for the shout-out for the react-webpack-rails-tutorial. Its sibling package is react_on_rails. |
@dhh wrote:
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:
|
@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:
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. |
@josepjaume We have the problem solved in React on Rails, such that "it just works". The solution involves:
This way, the correct file names are available on deployment. Any questions? |
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). |
@josepjaume this is also how the bower-rails gem resolved relative asset paths.
Has this option gotten any love recently? |
My Rails Version is 5.1.2 and Ruby 2.4 - I am using Yarn
Now, in application.css.scss I imported the 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? |
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 Thanks so much for your work on this feature!!! 🎉 |
@Schwad, there is a You can find a few things if you search for 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. |
@Schwad, also, if you run the |
Just came by to mass unsubscribe. |
@krushi123 You need to overwrite the |
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:
package.json
in new apps when--yarn
option is passedyarn install
afterbundle install
in new appspackage.json
node_modules
folder to asset paths