Skip to content

Conversation

paulrobertlloyd
Copy link
Contributor

This PR does 2 uncontroversial things:

  • Removes govuk-frontend asset files from being precompiled (we don’t use them)
  • Removes all .scss files from being precompiled (these are only used for the build:css script, which saves a compiled CSS file to /app/assets/build)

It also does 1 potentially controversial thing:

  • Removes the service worker scripts, related tasks and dependencies

This is dead code. The line that imports the service worked has been commented out since January 2024 (we currently use spreadsheets for offline working). The service worker scripts haven’t been updated alongside other changes so I’m wondering if any of it actually works now.

If we are happy removing the service worker, there is also documentation in the repo that says we use a service worker; should that be updated/removed too?

@paulrobertlloyd paulrobertlloyd requested a review from a team as a code owner August 26, 2025 10:42
@paulrobertlloyd paulrobertlloyd added the refactor Improving maintainability label Aug 26, 2025
@thomasleese thomasleese added this to the v3.4.0 milestone Aug 26, 2025
@misaka
Copy link
Contributor

misaka commented Aug 26, 2025

Happy to rip this out, I've been thinking it should be ripped out recently myself. I think we should take the 🪓 to the documentation at the same time is the right thing to do too.

@paulrobertlloyd paulrobertlloyd requested a review from a team as a code owner August 26, 2025 14:23
@paulrobertlloyd
Copy link
Contributor Author

paulrobertlloyd commented Aug 26, 2025

@misaka When you have a moment, could you take a look at 8906cfa? I think this removes all the relevant documentation, in the correct way (he says, walking around blindly in PlantUML).

I’ve left https://github.com/nhsuk/manage-vaccinations-in-schools/blob/main/docs/secure-offline-storage.md as that appears to document the different approaches we looked at, without suggesting any of them were implemented.

Copy link

@thomasleese thomasleese enabled auto-merge August 29, 2025 14:28
@thomasleese thomasleese merged commit 5b1259e into next Aug 29, 2025
13 checks passed
@thomasleese thomasleese deleted the assets-precompile branch August 29, 2025 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Improving maintainability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants