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 sprockets-rails gem to version 3.0.3 #2323

Merged
merged 2 commits into from
Mar 2, 2016

Conversation

erkde
Copy link
Contributor

@erkde erkde commented Mar 2, 2016

Using: bundle update sprockets-rails

Upgrades the sprockets-rails gem in smart answers from version 2.3.3 to version 3.0.3. The major release from 2.x to 3.x introduces breaking changes, including enabling asset digests by default (JS and CSS files will receive a checksum value in the in filename).

Sprockets-rails release note

With the smart answers regression tests saving the generated HTML in the repository, change config.assets.digest from the new default value of true to false to prevent all the artefacts changing as a result.

The upgrade to sprockets-rails in turn updates the sprockets gem from version 2.12.4 to 3.5.2. This major version brings a pretty significant list of changes, and notably brings smart answers inline with the major version 3.x which the 4-2-stable branch of Rails itself is developed towards.

See: https:/rails/rails/blob/4-2-stable/Gemfile#L18

Running smart answers locally with this change, no assets appear to be failing and the browser console is free of errors. The guide to upgrading sprockets from version 2.x to 3.x suggests:

For the most part, the application facing APIs have remained the same with the majority of the changes at the extension layer.

Smart Answers also seems unaffected by the following changes:

Prefer just foo.coffee and foo.scss

  • The smart answers SCSS files are already following this convention

Removed //= include directive

  • Seems unused within smart answers

Preview on Heroku

https://smart-answers-pr-2323.herokuapp.com/marriage-abroad

`bundle update sprockets-rails`

Upgrades the sprockets-rails gem in smart answers from version 2.3.3 to
version 3.0.3. The major release from 2.x to 3.x introduces breaking
changes, including enabling asset digests by default (JS and CSS files
will receive a checksum value in the in filename).
Disable the sprocket-rails config that adds a checksum value to JS
and CSS asset filenames in compiled assets which would change the
artefacts produced by the regression tests.
@floehopper floehopper self-assigned this Mar 2, 2016
@floehopper
Copy link
Contributor

I think it's worth explaining that we want to do this so we can do a simple bundle update rails to upgrade rails to v4.2.5.2. Give that this was the original intention, I might've been inclined just to upgrade rails and not to upgrade sprockets-rails at this point. It might be good to explain why you've chosen not to do that.

It's worth noting that this PR also upgrades sprockets from v2.12.4 to v3.5.2, a fairly significant change. It's hard to be sure that none of the changes listed in the changelog affect Smart Answers. However, I'd hope that sprockets-rails would shield us from most (if not all) of these changes.

If you haven't done so already, I think it would be worth precompiling the assets and running the app in production locally to check that everything looks OK - I'm not sure we have much test coverage of that side of things. Alternatively this could be checked in the environment that used to be called "preview" (is that now "integration"?).

Ideally this change would be deployed separately from any other upgrades, e.g. the upgrade to rails, in case there's a problem, so that it's easier to work out why the problem happened.

Otherwise, I can't see a reason not to merge this.

@erkde
Copy link
Contributor Author

erkde commented Mar 2, 2016

@floehopper I've updated the PR description, and will push the branch to integration to verify.

I think it's worth doing this upgrade before the Rails upgrade, as we're behind the version of sprockets that Rails is developed towards, and attempting to update Rails now to receive the latest security patches, forces sprockets-rails and sprockets to be to upgraded as well, as we saw in #2320

@floehopper
Copy link
Contributor

@erikse:

The PR description looks great.

Regarding your last comment, strictly speaking I don't think you're right in saying that updating rails forces sprockets-rails to be upgraded - I was able to upgrade rails without upgrading sprockets-rails by reverting the relevant parts of Gemfile.lock as per this comment i.e. there are no dependency constraints which force the upgrade of sprockets-rails.

However, your explanation in the PR description talks about wanting to come into line with future v4.2.x versions, which I think is a perfectly reasonable motivation.

👍

@floehopper floehopper added the LGTM label Mar 2, 2016
@ikennaokpala
Copy link
Contributor

@erikse LGTM

@erkde
Copy link
Contributor Author

erkde commented Mar 2, 2016

Ok, that's true - we can undo the upgrade to sprockets-rails and sprockets that occur as a result running bundle update rails, perhaps forces isn't the correct wording.

erkde pushed a commit that referenced this pull request Mar 2, 2016
Update sprockets-rails gem to version 3.0.3
@erkde erkde merged commit 330e52d into master Mar 2, 2016
@erkde erkde deleted the update_sprockets_rails_to_3_0_3 branch March 2, 2016 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants