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

Add support for Sprockets v4 to the DummyApp #3379

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Oct 11, 2019

🛑 BLOCKED BY rails/sprockets-rails#446

Description
Sprockets 4 introduced a mandatory configuration manifest at a specific location. We don't have the ability to set it at that specific location in the DummyApp we use for our specs.

I've opened rails/sprockets-rails#446 on sprockets-rails which should fix this issue. I'll leave this PR open (working with that branch) waiting to know if that PR will be accepted.

Update: Now an empty manifest will be written in the expected location inside spec/dummy during the dummy app setup.

Closes #3374, closes #3376

After merging this we need to revert This also reverts #3378.

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)

@kennyadsl kennyadsl self-assigned this Oct 11, 2019
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks. I think we should leave the file blank on purpose. Actually this file should be only necessary if people actually set Rails.application.assets.precompile += ['manifest.js'], but this is something to discuss with the Rails team.

@@ -87,6 +87,7 @@ class Application < ::Rails::Application
config.action_controller.include_all_helpers = false

if config.respond_to?(:assets)
config.assets.config_manifest = File.expand_path('dummy_app/assets/config/manifest.js', __dir__)
Copy link
Member

Choose a reason for hiding this comment

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

Love your PR 👍

Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

This is great!

@kennyadsl kennyadsl added the release:major Breaking change on hold until next major release label Dec 10, 2019
@kennyadsl kennyadsl added the type:bug Error, flaw or fault label Aug 24, 2022
@waiting-for-dev waiting-for-dev added the changelog:solidus_core Changes to the solidus_core gem label Aug 30, 2022
@waiting-for-dev waiting-for-dev removed the release:major Breaking change on hold until next major release label Dec 1, 2022
@elia elia self-assigned this Sep 25, 2023
@elia elia force-pushed the kennyadsl/sprockets-4-support branch from cc5a4f5 to 2fbe9c3 Compare September 25, 2023 17:06
@elia elia requested a review from a team as a code owner September 25, 2023 17:06
@elia elia added the hold On hold for a reason different from a breaking change label Sep 25, 2023
@elia elia force-pushed the kennyadsl/sprockets-4-support branch from 2fbe9c3 to c719179 Compare January 3, 2024 12:48
@elia elia removed their assignment Jan 5, 2024
@elia elia force-pushed the kennyadsl/sprockets-4-support branch 2 times, most recently from 8163a5f to 4024df7 Compare January 9, 2024 10:41
@github-actions github-actions bot added the changelog:repository Changes to the repository not within any gem label Jan 9, 2024
@elia elia removed changelog:solidus_core Changes to the solidus_core gem hold On hold for a reason different from a breaking change labels Jan 9, 2024
This is needed from Sprockets, since v4. It contains
all dependencies that needs to be compiled.

Co-Authored-By: Elia Schito <[email protected]>
@elia elia force-pushed the kennyadsl/sprockets-4-support branch from 4024df7 to 1a35ea5 Compare January 9, 2024 10:44
@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Jan 9, 2024
@elia elia self-assigned this Jan 9, 2024
Removing the dependency on sprockets < 4 rack is now unbounded, but
rack 3 is only supported from Rails 7.1 up.
@elia elia changed the title Add a DummyApp sprockets configuration manifest Add support for Sprockets v4 to the DummyApp Jan 9, 2024
@elia elia merged commit 7bab27b into solidusio:main Jan 9, 2024
12 checks passed
@elia elia deleted the kennyadsl/sprockets-4-support branch January 9, 2024 11:50
@elia elia mentioned this pull request Jan 9, 2024
@tvdeyen tvdeyen added the backport-v4.3 Backport this pull-request to v4.3 label Jun 28, 2024
Copy link

💔 All backports failed

Status Branch Result
v4.3 Backport failed because of merge conflicts

You might need to backport the following PRs to v4.3:
- Add a DummyApp sprockets 4 configuration manifest

Manual backport

To create the backport manually run:

backport --pr 3379

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

tvdeyen added a commit that referenced this pull request Jun 28, 2024
[v4.3]  Add support for Sprockets v4 to the DummyApp (backports #3379)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v4.3 Backport this pull-request to v4.3 changelog:repository Changes to the repository not within any gem changelog:solidus_core Changes to the solidus_core gem type:bug Error, flaw or fault
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error running test app on master (ManifestNeededError) Sprockets 4 compatibility
5 participants