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

Make `rails yarn:install` ignore dev dependencies #29851

Merged
merged 1 commit into from Jul 20, 2017

Conversation

Projects
None yet
5 participants
@printercu
Contributor

printercu commented Jul 19, 2017

Yarn rebuilds native packages on every yarn install call (issue yarnpkg/yarn#932). We have some native node packages in devDependencies to build custom fonts, and they got rebuilt on every deploy (30+ sec. instead of 2 sec.). This patch makes rails yarn:install which is used before assets:precompile ignore dev dependencies. bin/yarn still installs everything.

For anybody looking for workaround
Update bin/yarn with:

# ...
  begin
    no_dev = %w[production staging].include?(ENV['RAILS_ENV'])
    exec "yarnpkg #{ARGV.join(' ')} #{'--prod' if no_dev}"
@rails-bot

This comment has been minimized.

rails-bot commented Jul 19, 2017

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.

@eileencodes eileencodes merged commit e8eada4 into rails:master Jul 20, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eileencodes

This comment has been minimized.

Member

eileencodes commented Jul 20, 2017

Makes sense. Thanks for the PR! 👍

@printercu printercu deleted the printercu:patch-1 branch Jul 23, 2017

@FrankFang

This comment has been minimized.

FrankFang commented Aug 14, 2017

Good one. But I installed webpack/babel as devDeps so install --prod is not enought.

@printercu

This comment has been minimized.

Contributor

printercu commented Aug 14, 2017

I understand that it differs from frontend practices. In rails terms, rails assets:precompile is run in production environment, so webpack should be better placed in production dependencies. Like if it was gem, it would be out of group :development in Gemfile.

@FrankFang

This comment has been minimized.

FrankFang commented Aug 14, 2017

OK, it makes sense.

@lpil

This comment has been minimized.

lpil commented Apr 23, 2018

Typically npm build tools are considered development deps are they are not used in the production javascript bundle. With this convention this change breaks the build, or requires the user to move non-runtime deps into the runtime section.

@printercu

This comment has been minimized.

Contributor

printercu commented Apr 23, 2018

Does this tools run on production servers / build servers? If so, should not they be placed in production section?

@lpil

This comment has been minimized.

lpil commented Apr 23, 2018

We actually don't run this on production servers, we run it on CI and only the asset manifest is present on production servers. We believe that building of deployment artefacts is to be done before deployment, and no build tools should be present on said servers.

Doing this means that there is a much smaller attack vector, and it means we don't have to slowly compile the assets on every server when we deploy, instead it only happens once. It also makes zero-downtime deployments easier as we don't have to worry about the application knowing how to serve both versions of the assets during a rolling deployment.

To clarify, I think it's valid to build assets either in production or on CI, but this change forces the user to behave as if they are building in production, even if they are not.

@printercu

This comment has been minimized.

Contributor

printercu commented Apr 23, 2018

build servers

I mean CI too.

no build tools should be present on said servers

Should all other npm stuff be present on production servers then, if everything is already bundled?

Do you run assets:precompile with RAILS_ENV=production? Does this mean that all required stuff should be installed in production? Actually, I still use separate assets group in gemfile, and require them only for assets:precompile. Rails have removed this feature, and I have to do it in app. I'm just saying that there can be app-specific usage cases, which don't fit most of users.

@lpil

This comment has been minimized.

lpil commented Apr 23, 2018

On CI servers all assets are installed because tests run on CI servers as well.

The problem is that with this change the test/dev/build deps are now deleted from the CI server by this task.

@printercu

This comment has been minimized.

Contributor

printercu commented Apr 23, 2018

What is your suggestion to address problem described in #29851 (comment)?

@lpil

This comment has been minimized.

lpil commented Apr 23, 2018

I would only likely only install production node deps if the node env is production. i.e. NODE_ENV=production.

This is actually the default behaviour of yarn, so no code change was required here to fix your problem. Instead this change has broken other workflows.

@printercu

This comment has been minimized.

Contributor

printercu commented Apr 23, 2018

Am I getting you right: you are saying that RAILS_ENV=production rails assets:precompile should install yarn dev deps?

@lpil

This comment has been minimized.

lpil commented Apr 23, 2018

Yes, I don't think rails should specify which deps are installed as this is already managed by yarn/npm.

@printercu

This comment has been minimized.

Contributor

printercu commented Apr 23, 2018

Sorry, I don't think it should be default behaviour. One can provide pr to support overriding this, but i think NODE_ENV should be equal to RAILS_ENV by default.

PS. There is still bin/yarn which installs all deps.

@lpil

This comment has been minimized.

lpil commented Apr 23, 2018

but i think NODE_ENV should be equal to RAILS_ENV by default.

But that's not what is happening after this PR, setting the --production flag isn't the same as setting the NODE_ENV. :(

PS. There is still bin/yarn which installs all deps.

That's right, but yarn:install is implicitly called by other rakes tasks such as assets:precompile so any time we run assets:precompile it will delete all non-production deps before attempting to compile.

One can provide pr to support overriding this

Happy to contribute this :)

Would do you envision the override working? It needs to also work when the rake task is invoked implicitly by another task.

@printercu

This comment has been minimized.

Contributor

printercu commented Apr 23, 2018

it will delete all non-production deps

I was not aware of it. Docs say nothing about it, npm does not delete dev deps. Uh.

What about ENV['NODE_ENV'] ||= ENV['RAILS_ENV']? Will this work for most?

UPD.

node_env = ENV.fetch('NODE_ENV') { ENV['RAILS_ENV'] }
system({'NODE_ENV' => node_env}, 'bin/yarn ....')
@lpil

This comment has been minimized.

lpil commented Apr 23, 2018

npm does not but yarn cleans node_modules of dev deps if you pass the --production flag.

That would work I think! I'd just need to set "NODE_ENV" to "test" or something I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment