Skip to content

Commit

Permalink
Fixes generator tests
Browse files Browse the repository at this point in the history
- cascading test failures caused when no application.js file found during install generator. Because javascript convention changes between major rails versions, generating new sample app no longer reliably places a file at app/assets/javascripts/application.js. Now the install generator will touch that location if detect_js_format falls through, though there may be a better way to do this.
  • Loading branch information
aseroff committed Jul 23, 2024
1 parent 7821ed4 commit 5f69067
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 1 deletion.
2 changes: 2 additions & 0 deletions lib/generators/serviceworker/install_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ def detect_js_format

return [ext, "//="]
end
FileUtils.touch javascripts_dir("application.js")
['.js', "//="]
end

def detect_layout
Expand Down
2 changes: 1 addition & 1 deletion test/serviceworker/install_generator_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class ServiceWorker::InstallGeneratorTest < ::Rails::Generators::TestCase
end

assert_file "app/assets/javascripts/application.js" do |content|
assert_match(%r{\n//= require serviceworker-companion}, content)
assert_match(%r{//= require serviceworker-companion}, content)
end
end

Expand Down

5 comments on commit 5f69067

@aseroff
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be an undertaking, but the best option would probably be to refactor detect_js_format to look for Rails 6 webpacker and Rails 7 importmap style javascript setups.

@rossta
Copy link
Owner

@rossta rossta commented on 5f69067 Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we save some initial effort here by making this step manual to start? I’m thinking we drop the step of trying to find the right place to require/import serviceworker-companion and add a step in the README and/or instructions in the output of the generator. What do you think?

@rossta
Copy link
Owner

@rossta rossta commented on 5f69067 Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this change, I’ve cherry-picked it onto #128

@aseroff
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we save some initial effort here by making this step manual to start? I’m thinking we drop the step of trying to find the right place to require/import serviceworker-companion and add a step in the README and/or instructions in the output of the generator. What do you think?

That seems like a very reasonable way to handle it to me.

@aseroff
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like this?

Please sign in to comment.