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

Update compute_asset_path to Rails 4 signature #214

Merged
merged 1 commit into from Sep 27, 2017

Conversation

@fofr
Copy link
Contributor

@fofr fofr commented Aug 14, 2017

Fixes #213

The JasmineRails::OfflineAssetPaths monkey patch of compute_asset_path has a Rails 3 signature:
https://github.com/searls/jasmine-rails/blob/master/lib/jasmine_rails/offline_asset_paths.rb#L9

This was updated from 3 arguments to 2 in Rails 4:
Rails 3: https://apidock.com/rails/ActionView/AssetPaths/compute_public_path
Rails 4: https://apidock.com/rails/ActionView/Helpers/AssetUrlHelper/compute_asset_path

If JasmineRails::OfflineAssetPaths.disabled is true, then the method is meant to shortcut and run the original super method. This gave an error like:

ActionView::Template::Error: wrong number of arguments (given 3, expected 1..2)

cc @h-lame

@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 14, 2017

Test failures look unrelated.

@searls
Copy link
Owner

@searls searls commented Aug 14, 2017

We still support Rails 3, though, so could you put this method definition behind a branch that checks version?

@fofr fofr force-pushed the alphagov:fix-monkey-patch branch from 3aabc09 to 9e8794f Aug 14, 2017
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 14, 2017

@searls I've updated in 9e8794f to maintain Rails 3 support.

Thought about a switching branch based on Rails version but there was already a Rails 3 entry point on line 30. I've updated this to be explicit about the Rails 3 arguments (they match the old methods before #204).

For Rails 3:

  • If offline assets are disabled it will call super and pass in the original arguments, including any dir value that's been set
  • If enabled it will call compute_asset_path and ignore the unused dir variable
fofr added a commit to alphagov/government-frontend that referenced this pull request Aug 15, 2017
Use patch of jasmine-rails until upstream PR is accepted and versioned:
searls/jasmine-rails#214

Issue documented here:
searls/jasmine-rails#213
fofr added a commit to alphagov/government-frontend that referenced this pull request Aug 15, 2017
Use patch of jasmine-rails until upstream PR is accepted and versioned:
searls/jasmine-rails#214

Issue documented here:
searls/jasmine-rails#213
@fofr
Copy link
Contributor Author

@fofr fofr commented Aug 22, 2017

@searls Is there anything else I need to update following the changes added in 9e8794f?

@searls
Copy link
Owner

@searls searls commented Aug 22, 2017

@timdiggins
Copy link
Contributor

@timdiggins timdiggins commented Sep 18, 2017

@fofr I think the build probs now fixed by #219 so rebasing on master should sort it (ref: https://travis-ci.org/timdiggins/jasmine-rails/builds/276747009)

Fixes #213

The JasmineRails::OfflineAssetPaths monkey patch of
`compute_asset_path` has a Rails 3 signature:
https://github.com/searls/jasmine-rails/blob/master/lib/jasmine_rails/of
fline_asset_paths.rb#L9

This was updated from 3 arguments to 2 in Rails 4:
https://apidock.com/rails/ActionView/AssetPaths/compute_public_path
vs
https://apidock.com/rails/ActionView/Helpers/AssetUrlHelper/compute_asse
t_path

If JasmineRails::OfflineAssetPaths.disabled is true, then the method is
meant to shortcut and run the original super method. This gives an
error like:

ActionView::Template::Error: wrong number of arguments (given 3,
expected 1..2)

* Update `compute_asset_path` to Rails 4 signature
* Use explicit arguments for `compute_public_path`
* Ignore unused `dir` variable from Rails 3
@fofr fofr force-pushed the alphagov:fix-monkey-patch branch from 9e8794f to a35b53f Sep 18, 2017
@fofr
Copy link
Contributor Author

@fofr fofr commented Sep 18, 2017

@timdiggins Thanks Tim, rebased and tests are now passing.

@searls
Copy link
Owner

@searls searls commented Sep 27, 2017

Sorry for the delay in reviewing this. Looks good to me.

@searls searls merged commit 00da363 into searls:master Sep 27, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sihugh added a commit to alphagov/government-frontend that referenced this pull request Jan 12, 2018
We had been using a monkey-patched version because of an issue with the
[OfflineAssetPaths monkey patch](searls/jasmine-rails#213)

This was fixed in [PR #214](searls/jasmine-rails#214)
and released in [v0.14.3](https://github.com/searls/jasmine-rails/blob/832bf7f5f203f09801b395e5c95b5d23df83a3bf/CHANGELOG.md#v0143-2017-09-27)
so we should be OK to return to the rubygems version.
alextea added a commit to alphagov/government-frontend that referenced this pull request Mar 22, 2018
We had been using a monkey-patched version because of an issue with the
[OfflineAssetPaths monkey patch](searls/jasmine-rails#213)

This was fixed in [PR #214](searls/jasmine-rails#214)
and released in [v0.14.3](https://github.com/searls/jasmine-rails/blob/832bf7f5f203f09801b395e5c95b5d23df83a3bf/CHANGELOG.md#v0143-2017-09-27)
so we should be OK to return to the rubygems version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.