Fix bin/setup modification failure in installation #229
Conversation
ahangarha
force-pushed
the
improve-robustness-in-modifying-bin-setup
branch
2 times, most recently
from
58f5df3 to
4292001
Compare
ahangarha
changed the title
lib/install/template.rb
Outdated
| else | ||
| say <<~MSG, :red | ||
| It seems your `bin/setup` file doesn't have the expected content. | ||
| Please review the file and manually add `system! "bin/yarn"`. | ||
| MSG | ||
| end | ||
| end |
There was a problem hiding this comment.
@Judahmeek @justin808
What about adding these lines at the end of the file if we couldn't find the pattern? The job of this step in the generator is to ensure we have system!("bin/yarn") in the setup file. So why should we ask the user to do it manually?
I think it would be good to print out some text and say thought we have updated the setup file, but the format of the file was not what we expected. This way, we can ask the developer to review the content of this file and ensure everything is right in the file.
It is not a very important thing to be worried about but I think if we are updating this part, we do it in the right way. Is my proposal the right way? What do you think?
ahangarha
force-pushed
the
improve-robustness-in-modifying-bin-setup
branch
from
4292001 to
21e33ba
Compare
ahangarha
changed the title
|
@ahangarha I'll merge after you update the CHANGELOG. However, I'm not convinced this modification regarding bin/yarn is even needed longer term. |
Due to different content of bin/setup in different Rails versions and
installation date, the earlier version of the generator couldn't modify
this file properly in all circumstances.
This commit handles the following scenarios:
- If there is a commented line for `system('bin/yarn')`, uncomment it.
- Otherwise, regardless of using single or double quotations in
different Rails versions, add `system! 'bin/yarn'` after the intended
line.
ahangarha
force-pushed
the
improve-robustness-in-modifying-bin-setup
branch
from
21e33ba to
4eb8964
Compare
@justin808 I updated the changelog. |
| @@ -38,11 +38,27 @@ | |||
|
|
|||
| if (setup_path = Rails.root.join("bin/setup")).exist? | |||
| say "Run bin/yarn during bin/setup" | |||
| insert_into_file setup_path.to_s, <<-RUBY, after: %( system("bundle check") || system!("bundle install")\n) | |||
|
|
|||
| if File.read(setup_path).match? Regexp.escape(" # system('bin/yarn')\n") | |||
There was a problem hiding this comment.
[] please extract system('bin/yarn') to a const, also why it's system('bin/yarn') and system**!**('bin/yarn') in other places?
[] please add a warn, that the file has been modified.
[] (optional), add a comment what and why we are uncommenting, unless this will not be intuitive from the log above.
There was a problem hiding this comment.
- May you elaborate on the extraction of
system('bin/yarn')? If it is for DRY, I don't think this is a good place to do so because of having a different format in some parts. And then if I extract it, I think we get into a readability issue. - Regarding
systemandsystem!, rails 5 usedsystemonly. I didn't check if it was intended. Since we use!for it, it is uncommented and changed tosystem!. Do you think there was a particular reason behind usingsystemin Rails 5? - Do we need to warn the user about this? I think the message generated by
gsub_fileandinsert_into_fileis sufficient and enough informative. - Well, as per my understanding we want to run
yarn installwhen we run./bin/setupscript. In rails 5 and 6, they had this item in the file. In Rails 5 it was commented and without!and in Rails 6 it was uncommented with!. In Rails 7 they removed it entirely (even if one specifies--javascript=webpack).
There was a problem hiding this comment.
Regarding system and system!, rails 5 used system only. I didn't check if it was intended. Since we use ! for it, it is uncommented and changed to system!. Do you think there was a particular reason behind using system in Rails 5?
@ahangarha thanks, this makes sense. Could you please then just some comments, or extract a function, so the code will be easier to understand?
| RUBY | ||
| else | ||
| say <<~MSG, :red | ||
| It seems your `bin/setup` file doesn't have the expected content. |
There was a problem hiding this comment.
Do we have a section in the docs, which might be referenced in this warn message?
There was a problem hiding this comment.
I don't think so. But also see my comment here:
What about adding these lines at the end of the file if we couldn't find the pattern? The job of this step in the generator is to ensure we have
system!("bin/yarn")in thesetupfile. So why should we ask the user to do it manually?
I didn't get any response on it. Do you have any?
There was a problem hiding this comment.
@ahangarha I think it's a good idea to add both in this case and keep the message, which will give heads-up to the user.
Due to different content of
bin/setupin different Rails versions, the earlier version of the generator couldn't modify this file properly in all circumstances.This commit handles the following scenarios:
system('bin/yarn'), uncomment it.system! 'bin/yarn'after the intended line.I have manually checked the impact of this PR on the following Rails versions:
5.2.8: where we already have commentedsystem('bin/yarn')6.1.7: where we havesystem('bundle check') || system!('bundle install')7.0.4: where we havesystem("bundle check") || system!("bundle install")closes #226