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

Clarify customizability of config/assets.js #293

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robyurkowski
Copy link

Previously, there was a line that was just await assets.run() that was not commented, and a commented block that was suggested for customization. As a result, it was possible that the initial line would be left when enabling the block, which causes a double-build and can also cause a lot of confusion once some customization is added.

Given that the default customization function doesn't do anything, the computational cost of just executing it seems acceptable to simplify the setup and reduce the chance of frustration.

(I know this because I ran into this issue while adding Tailwind and PostCSS, and wound up being confused why builds were failing to find Tailwind. Real PEBKAC, but I hope that this would help save someone else the grief. I also expanded the comments on the file to try and tip people off that Hanami adds some configuration so that they don't also just accidentally clobber it like I did.)

Previously, there was a line that was just `await assets.run()` that was not commented, and a commented block that was suggested for customization. As a result, it was possible that the initial line would be left when enabling the block, which causes a double-build and can also cause a lot of confusion once some customization is added. 

Given that the default customization function doesn't do anything, the computational cost of just executing it is almost certainly acceptable to simplify the setup and reduce the chance of frustration.
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.

1 participant