administrate icon indicating copy to clipboard operation
administrate copied to clipboard

Run diff-check on JS/CSS to catch updates

Open nickcharlton opened this issue 1 year ago • 3 comments

In #2673, we discussed catching changes in the builds directory as being quite annoying in development. This can happen when people contribute changes (and should probably also commit their build changes to make future development easier), plus also when dependencies change.

Catching this with diff-check should mean that we see this at the point of introduction, rather than later on.

nickcharlton avatar Oct 08 '24 15:10 nickcharlton

I do feel like it's not very clear what you should do with that failure, maybe it needs a custom message or something to make it clearer.

nickcharlton avatar Oct 08 '24 15:10 nickcharlton

Is there any reason not to just add the build files to .gitignore as suggested by @goosys https://github.com/thoughtbot/administrate/pull/2673#issuecomment-2394008339

littleforest avatar Oct 09 '24 00:10 littleforest

Just to confirm: this is intended to force contributors to check in any changes to compiled assets, right? In which case, it gets my vote, unless I'm missing a use case somewhere :slightly_smiling_face:

@littleforest - The issue I'm seeing is that any assets missing from the repo are not available when pulling the gem from Git (eg: gem "administrate", git: ....

pablobm avatar Oct 19 '24 14:10 pablobm

this is intended to force contributors to check in any changes to compiled assets

Yeah, that's my thinking. If you make changes, your build will fail and prompt you to run the build.

A little while after I did this, it occurred to me that it solves another problem quite nicely, too: it stops us from having to make sure the prerelease Administrate demo is compiled correctly.

On this though, next I'd like to make sure it's got a better suggestion of what to do. Then once we've cut a new release we can merge this in.

nickcharlton avatar Oct 21 '24 13:10 nickcharlton

I figured out why this kept failing, but I couldn't locally and just merged in #2710. It was a tiny change by JS bundling standards, and caused by how npm actually works (badly…).

Anyway, the solution is to do something like this:

git clean -fdx
bin/setup
yarn install
yarn build
yarn build:css

diff-check doesn't actually have a way to prompt what to do yet. I'd like to add that, but I'd also like to merge this sooner than later, so I'm going to do that.

nickcharlton avatar Nov 19 '24 11:11 nickcharlton