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

Uncomments bin/yarn in bin/setup for webpacker #40265

Merged

Conversation

pftg
Copy link
Contributor

@pftg pftg commented Sep 21, 2020

Summary

By adding the default requiring of webpacker, bin/setup requires a manual update in order to do the cold setup on a new instance.

Other Information

If users do not use bin/yarn to install packages, then they still require to delete bin/yarn line in order to remove redundant code.

If we already add bin/yarn, then run it on bin/setup should not increase any confusion.
And at the same time, we reduce confusion when we run bin/setup without node_modules will not setup valid application.

Cosmetic Changes

There is some cosmetic change which makes code consistent: system('bin/yarn') => system! 'bin/yarn'

@rails-bot rails-bot bot added the railties label Sep 21, 2020
@pftg pftg force-pushed the run-bin-yarn-on-setup-by-default branch from 9431c4b to 572998f Compare September 22, 2020 16:20
@pftg
Copy link
Contributor Author

pftg commented Sep 22, 2020

There is an issue with listen gem on buildkite and with setup test env for master code, so decided to reschedule it. But other pipelines work fine.

Will check other failed tests

@pftg pftg force-pushed the run-bin-yarn-on-setup-by-default branch from 572998f to 1c5c480 Compare September 22, 2020 17:25
@pftg pftg changed the title Uncomments bin/yarn in bin/setup for webpacker WIP: Uncomments bin/yarn in bin/setup for webpacker Sep 22, 2020
@pftg pftg force-pushed the run-bin-yarn-on-setup-by-default branch from 1c5c480 to 52ce567 Compare September 22, 2020 17:35
@pftg pftg changed the title WIP: Uncomments bin/yarn in bin/setup for webpacker Uncomments bin/yarn in bin/setup for webpacker Sep 22, 2020
@pftg
Copy link
Contributor Author

pftg commented Sep 22, 2020

Fixed all tests related to my changes. There is a flaky test in the master https://buildkite.com/rails/rails/builds/71715#d1f5e263-fef9-449c-b701-45b576f1b05c

Could someone check my PR and provide some feedback?

By adding installing of webpacker, `bin/setup` requires manual update in order to do cold setup. This adds extra steps to generate app and setup it.
@pftg pftg force-pushed the run-bin-yarn-on-setup-by-default branch from 52ce567 to 133805f Compare September 22, 2020 19:01
@rafaelfranca rafaelfranca merged commit 48b7043 into rails:master Sep 22, 2020
@pftg pftg deleted the run-bin-yarn-on-setup-by-default branch September 22, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants