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.

Sign up for GitHub

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

Jump to bottom

Fix Sprockets 4 support for extensions #3373

Merged
merged 1 commit into from
Oct 23, 2019
Merged

Fix Sprockets 4 support for extensions #3373

merged 1 commit into from
Oct 23, 2019

Conversation

aldesantis
Copy link
Member

@aldesantis aldesantis commented Oct 10, 2019 •
edited
Loading

Description

This fixes an issue with extension tests failing due to Sprockets 4 requiring an app/assets/images directory to be present.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)

@aldesantis aldesantis changed the title Create app/assets/images on install for Sprockets 4 compatibility Sprockets 4 compatibility Oct 10, 2019
@aldesantis aldesantis changed the title Sprockets 4 compatibility WIP: Sprockets 4 compatibility Oct 10, 2019
@kennyadsl kennyadsl mentioned this pull request Oct 11, 2019
Merged
4 tasks
@kennyadsl
Copy link
Member

We'll need this PR to fix all extensions specs, but if #3379 will be merged only. Otherwise, we can close this PR and backport the Sprockets 3 lock to all the supported Solidus versions.

@aldesantis
Copy link
Member Author

I'll close this in the meantime since we've locked Solidus to Sprockets 3. If rails/sprockets-rails#446 is merged, Solidus may become compatible with Sprockets 4, in which case I'll reopen it.

@aldesantis aldesantis closed this Oct 23, 2019
@aldesantis aldesantis reopened this Oct 23, 2019
@aldesantis
Create app/assets/images on install for Sprockets 4 compatibility
b6713b2 
Sprockets 4 uses manifest files[1], and the default manifest.js
links `app/assets/images`. To prevent it from crashing due to the
directory not being present, the easiest solution is to create an
empty `app/assets/images` directory.

[1]: https://github.com/rails/sprockets/blob/master/UPGRADING.md#manifestjs
@aldesantis aldesantis force-pushed the aldesantis/bugfix/sprockets-4 branch from 20a3b3f to b6713b2 Compare October 23, 2019 17:20
@aldesantis aldesantis changed the title WIP: Sprockets 4 compatibility Fix Sprockets 4 support for extensions Oct 23, 2019
@aldesantis
Copy link
Member Author

aldesantis commented Oct 23, 2019 •
edited
Loading

Reopening this, since:

Sorry about the confusion!

Note that this will also need to be backported to all currently supported Solidus version branches. We won't need to release new versions, since it's a dev-only change and extensions use branches rather than tags.

jacobherrington
jacobherrington reviewed Oct 23, 2019
View reviewed changes
Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

I am happy with merging this, but I'd like to see a comment explaining why we need to do this or at least linking back to this PR.

core/lib/generators/spree/install/install_generator.rb Show resolved Hide resolved
@kennyadsl kennyadsl merged commit 4051fd6 into solidusio:master Oct 23, 2019
@elia elia mentioned this pull request Dec 10, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobherrington jacobherrington jacobherrington approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aldesantis @kennyadsl @jacobherrington