hanami
Clarify customizability of config/assets.js
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.)
thank you for this @robyurkowski 🙂 ! I agree I think this is a nice guard rail to have with minimal cost.
there are a few failing tests, do you mind fixing those up? then we can get this merged. Thanks!
+1 happy to see this change go in! Let's use standard //-style comments rather than the special JS multi-line comments, though. These files will sit by default in a predominantly Ruby-flavoured codebase, so I think that using a consistent mode of commenting is a positive.
@kyleplump I'm happy to see you manage this the rest of the way from here :)
No probs. I'll try to get to it in the next few days, but feel free to poke me if I haven't.
@robyurkowski Made some tweaks and merged this one in. Thanks for doing this, I think it's a real improvement!
Hey, thanks for cleaning this up and merging, Tim, and apologies I didn't get to it sooner!
One of the intents with this PR was to additionally hint that Hanami adds some configuration to esbuild such that you can't clobber esBuildOptions without consequence. In particular, the line You can modify or add to Hanami's preset esbuild options here served this function, but even that feels like it's not as clear as it could be.
Is there a way to preserve that line, make clear that esbuildOptions has some modifications, or even inline those modifications so that devs aren't accidentally blowing things up?
For example, one of the things that I first did was add plugins: [ postCSSPlugin({ ... }) ], but that overwrote the existing plugin config and it seems like that would be very easy to do without an indication that plugins is already defined. Debugging it was a bit tricky, as I recall, and I wound up reading through source to figure out what was going on.
Any suggestions? If that feels like something that's worth an ounce of prevention, I'm happy to help where I can, though spring is a bit of a crazy season on the farm, so I might be a bit slow. :)