Remove Grunt: consolidate build on webpack + npm scripts#594
Merged
Conversation
Webpack now handles both JS and SCSS via sass-loader + mini-css-extract-
plugin (with webpack-remove-empty-scripts to drop the empty styles JS
chunk). Output is reorganized under dist/{js,styles}/ with a single
combined manifest at dist/manifest.json; AppContainer loads it and
webpackAsset middleware exposes both webpackAssetJS and webpackAssetCSS
helpers. The default and newshub pug templates now link CSS via the
manifest, dropping the ?version=Date.now() cache-busting hack in favor
of content hashes.
The dev runner becomes `yarn dev` (webpack --watch + nodemon under
concurrently); prod build becomes `yarn build`. Dockerfile-dev CMD and
the prod Dockerfile build step are updated accordingly. Removes 8
grunt-* devDeps, Gruntfile.js, and the grunt/ folder.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR migrates the build system from Grunt to webpack with native npm scripts. Webpack now extracts SASS stylesheets as hashed CSS files, the backend loads a unified manifest instead of JS-specific manifests, and asset middleware provides separate helpers for JS and CSS entry points. Development uses nodemon and concurrent npm scripts instead of Grunt tasks. ChangesBuild System Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/dependency-injection/AppContainer.js`:
- Around line 12-13: The module currently does eager one-time loading of
dist/manifest.json into the webpackManifest constant at import time causing
startup failures when the manifest is missing and stale assets after rebuilds;
change this to a lazy loader function (e.g., getWebpackManifest or loadManifest)
used by the middleware that currently references webpackManifest so the manifest
is read on demand (and in development optionally cached/invalidated between
requests), and handle/read errors gracefully so startup doesn't crash when
manifest is not yet written.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c50ccd26-10f3-4a79-a006-3f4628dd9419
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (17)
.gitignoreDockerfileDockerfile-devGruntfile.jsgrunt/concurrent.jsgrunt/nodemon.jsgrunt/run.jsgrunt/sass.jsgrunt/watch.jsnodemon.jsonpackage.jsonsrc/backend/AppKernel.jssrc/backend/dependency-injection/AppContainer.jssrc/backend/middleware/webpackAsset.jssrc/backend/templates/layouts/default.pugsrc/backend/templates/views/newshub.pugwebpack.config.js
💤 Files with no reviewable changes (7)
- grunt/run.js
- grunt/watch.js
- grunt/sass.js
- grunt/concurrent.js
- grunt/nodemon.js
- Gruntfile.js
- .gitignore
Comment on lines
+12
to
+13
| const webpackManifest = JSON.parse( | ||
| fs.readFileSync('dist/manifest.json', 'utf8') |
There was a problem hiding this comment.
Avoid eager, one-time manifest loading.
Line 12 loads dist/manifest.json at module import and Line 23 reuses that static object forever. With yarn dev (webpack watch + nodemon in parallel), this can crash startup if the manifest is not written yet, and it can serve stale hashed assets after rebuilds.
Suggested direction
+const path = require('path')
const fs = require('fs')
-const webpackManifest = JSON.parse(
- fs.readFileSync('dist/manifest.json', 'utf8')
-)
+const loadWebpackManifest = () =>
+ JSON.parse(
+ fs.readFileSync(path.resolve(process.cwd(), 'dist/manifest.json'), 'utf8')
+ )
module.exports.appContainer = function (appConfig) {
const container = new ContainerBuilder()
- container.setParameter('webpackManifest', webpackManifest)
+ container.setParameter('webpackManifest', loadWebpackManifest)Then have the middleware call the loader (at least in development) instead of capturing a frozen manifest snapshot.
Also applies to: 23-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/dependency-injection/AppContainer.js` around lines 12 - 13, The
module currently does eager one-time loading of dist/manifest.json into the
webpackManifest constant at import time causing startup failures when the
manifest is missing and stale assets after rebuilds; change this to a lazy
loader function (e.g., getWebpackManifest or loadManifest) used by the
middleware that currently references webpackManifest so the manifest is read on
demand (and in development optionally cached/invalidated between requests), and
handle/read errors gracefully so startup doesn't crash when manifest is not yet
written.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sass-loader+mini-css-extract-plugin+webpack-remove-empty-scripts). Output reorganized underdist/{js,styles}/with a single combined manifest atdist/manifest.json.webpackAssetmiddleware exposeswebpackAssetJS(entry)and a newwebpackAssetCSS(entry); the default layout andnewshub.pugnow resolve their stylesheet through the manifest, replacing the?version=Date.now()cache-busting hack with proper content hashes.yarn dev—webpack --watch+nodemonstarted in parallel viaconcurrently. Prod build isyarn build.Dockerfile-devCMD and the prodDockerfilebuild step updated accordingly.Gruntfile.js, thegrunt/folder, and 8grunt-*devDeps. Net diff: +202 / -1457.Why
Reduces the build pipeline from two parallel runners (Grunt + webpack) down to one, removes EOL-tier tooling, and unifies file-watching. Identified as the highest-impact "quick cleanup" item in the stack assessment.
Test plan
yarn buildproducesdist/js/*.[hash].js,dist/styles/styles.[hash].css, and a manifest with both*.jsandstyles.csskeysyarn lintcleanyarn test— 71/71 (integration tests boot AppKernel and hit/+/newshub, exercising the new CSS manifest path end-to-end)yarn dev— edit a.sassfile, confirm CSS rebuilds; edit a backend file, confirm nodemon restartsgruntanywhere outside this repo (faf-stack, deploy configs)🤖 Generated with Claude Code
Summary by CodeRabbit
yarn buildandyarn devrespectively.