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 assets.precompile config to Engine #1904

Merged
merged 3 commits into from
Feb 26, 2021
Merged

Conversation

Spone
Copy link
Contributor

@Spone Spone commented Feb 12, 2021

This is a PR to apply the fix suggested by #1816 (comment)

Close #1816

@pablobm
Copy link
Collaborator

pablobm commented Feb 18, 2021

This looks good to me. I tested it in the following situations:

  • Several versions of Rails pre 6.0.
  • Rails 6.0.
  • Rails 6.0 without Sprockets.
  • Rails 6.0 without Webpacker.

Rails pre-6.0 is not affected as it doesn't have Webpacker. For the other three, this PR removes the issue of having to manually update manifest.js when Administrate is installed, and it appears to work well.

My tests were not very comprehensive, but between this and the test suite passing (which includes some JS-dependant code), I feel confident enough that this is a good change.

@nickcharlton Any thoughts? Double checking before I merge as this might be an important change.

@nickcharlton
Copy link
Member

This was a bit fun to test! I tried with a script I have to make demo apps but couldn't replicate the original problem but tried it on my current client project where I could. My feeling is that I'm happy with that enough, because clearly this change doesn't break the old implementation and does work where it should.

Thanks for this! I'm going to merge this now.

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.

Sprockets 4 and Webpacker Config/Wiki
3 participants