Skip to content

Fix webpacker asset configs for external apps#8107

Merged
leio10 merged 6 commits intodecidim:developfrom
mainio:fix/webpacker-asset-configs-for-external-apps
Jun 8, 2021
Merged

Fix webpacker asset configs for external apps#8107
leio10 merged 6 commits intodecidim:developfrom
mainio:fix/webpacker-asset-configs-for-external-apps

Conversation

@ahukkanen
Copy link
Copy Markdown
Contributor

@ahukkanen ahukkanen commented Jun 6, 2021

🎩 What? Why?

As described at #8103, the asset configurations do not work for external apps outside of the Decidim main repo.

This is an oversight in the original implementation (#8066) where the webpacker override load path was added through the core gemspec. Obviously, this gemspec is not loaded in the actual applications.

This fixes the issue by applying the override load path directly to the webpacker binstubs during the webpacker install command, as that is the only place where this can be done. In addition, this change broke the assets:precompile task in some occasions which also needs to know the watch paths (additional paths) from the overridden configuration in order to calculate the asset hash to decide whether the assets need to be recompiled or not.

📌 Related Issues

Testing

See #8066 + try out the assets:precompile task which should be already in the CI.

📋 Checklist

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

leio10
leio10 previously approved these changes Jun 7, 2021
Copy link
Copy Markdown
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll give a try to double check it. In the meanwhile, can you rebase it to the last develop commit to fix the generator tests? Thanks!

@ahukkanen
Copy link
Copy Markdown
Contributor Author

@leio10 Rebased with develop and fixed the issue you mentioned.

@ahukkanen ahukkanen requested a review from leio10 June 8, 2021 05:56
@leio10 leio10 merged commit a65a1c3 into decidim:develop Jun 8, 2021
@ahukkanen ahukkanen deleted the fix/webpacker-asset-configs-for-external-apps branch June 8, 2021 07:53
@andreslucena andreslucena added type: internal PRs that aren't necessary to add to the CHANGELOG for implementers and removed target: developer-experience labels Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in-review module: core type: fix PRs that implement a fix for a bug type: internal PRs that aren't necessary to add to the CHANGELOG for implementers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Webpacker::Manifest::MissingEntryError after PR #8066 being updated

3 participants