Skip to content

[codex] finish dependency consolidation and restore CI#1402

Merged
justin808 merged 7 commits intomainfrom
jg-codex/finish-pr-1385
Apr 3, 2026
Merged

[codex] finish dependency consolidation and restore CI#1402
justin808 merged 7 commits intomainfrom
jg-codex/finish-pr-1385

Conversation

@justin808
Copy link
Copy Markdown
Collaborator

This replaces #1385 from a repo-owned branch so the dependency batch can run under the repository's normal pull request workflows.

What changed

Root cause

Updating node-polyfill-webpack-plugin to the current major fixed the asset regeneration/install path, but that plugin version no longer provides process by default. The generated react-server bundle then raised TypeError: Cannot read properties of undefined (reading 'env') in the Ruby test matrix.

Impact

Validation

  • env PACKAGE_JSON_FALLBACK_MANAGER=yarn_classic mise x ruby@2.7.8 node@20.19.0 -- bundle _2.4.9_ exec ruby -Itest test/react/server_rendering/exec_js_renderer_test.rb
  • env PACKAGE_JSON_FALLBACK_MANAGER=yarn_classic mise x ruby@2.7.8 node@20.19.0 -- bundle _2.4.9_ exec rake test
  • GitHub Actions on the source branch were green for the previously failing base, propshaft, and shakapacker jobs before opening this replacement PR

Notes

claude and others added 7 commits February 15, 2026 07:58
Combines all 12 open dependabot PRs into a single update:

Root (yarn.lock):
- bump serialize-javascript from 6.0.1 to 6.0.2
- bump nanoid from 3.3.6 to 3.3.11
- bump postcss from 8.4.25 to 8.5.6
- bump webpack from 5.83.1 to 5.104.1

test/dummy:
- bump js-yaml from 4.1.0 to 4.1.1
- bump node-forge from 1.3.1 to 1.3.3
- bump brace-expansion from 1.1.11 to 1.1.12
- bump webpack-dev-server from 4.13.1 to 5.2.1
- bump shakapacker from 7.2.0 to 9.5.0
- bump webpack from 5.76.3 to 5.104.1

react-builds:
- bump webpack from 5.85.0 to 5.104.1
- bump qs from 6.11.2 to 6.14.2

Consolidates PRs: #1370, #1371, #1372, #1373, #1374, #1375, #1376, #1379, #1381, #1382, #1383, #1384

https://claude.ai/code/session_01VRfTFW5v4wn4nwMyyyxLYv
Rebuild react and ujs assets with updated webpack to ensure
built files match the updated dependencies.

https://claude.ai/code/session_01VRfTFW5v4wn4nwMyyyxLYv
@claude
Copy link
Copy Markdown

claude bot commented Apr 3, 2026

PR Review

Thanks for the detailed PR description and for moving this to a repo-owned branch to unblock the CI workflows. The root cause analysis is clear and the fix is well-targeted.


What looks good

  • process polyfill fix is correct. node-polyfill-webpack-plugin v3+ removed process from its default aliases, so adding additionalAliases: ['process'] in webpack.config.js is the right fix for the TypeError: Cannot read properties of undefined (reading 'env') error in ExecJS server rendering.
  • shakapacker.yml configuration correctly adds the explicit assets_bundler: webpack and javascript_transpiler: babel keys that shakapacker 9 now requires (these were implicit defaults in earlier versions).
  • Pre-validation noted in the PR description — good that the source branch was green on CI before opening this replacement PR.
  • Both lock files updated consistently with the dependency changes.

Questions / potential concerns

1. Shakapacker 7 → 9 (major version skip)

The jump skips v8 entirely. While CI being green is encouraging, it would be helpful to note whether any shakapacker changelog entries for v8 or v9 introduced breaking changes that affected other config files beyond shakapacker.yml. The two added keys appear to be the only required migration step for this repo's usage — is that confirmed from the shakapacker migration guide?

2. webpack-cli stays at v4 while webpack-dev-server jumps to v5

- "webpack-dev-server": "^4.9.2"
+ "webpack-dev-server": "^5.2.3"
  "webpack-cli": "^4.9.2"   # unchanged

webpack-dev-server v5 dropped support for webpack-cli v4 in some configurations. The test suite may not exercise dev-server startup directly, so this could silently break local development workflows. Worth either upgrading webpack-cli to v5 or confirming v4 is still supported by webpack-dev-server v5.

3. css-minimizer-webpack-plugin not upgraded alongside other CSS tooling

  "css-loader": "^5.2.7" → "^7.1.4"
  "mini-css-extract-plugin": "^1.6.2" → "^2.10.2"
  "css-minimizer-webpack-plugin": "^2.0.0"   # unchanged

css-minimizer-webpack-plugin v2 is quite old relative to the current v7. It may still work with the updated css-loader/mini-css-extract-plugin, but leaving it at v2 seems like an oversight given the rest of the CSS tooling is being updated. Not a blocker, but worth a follow-up.

4. Committed generated bundles

The changes to lib/assets/react-source/development/react-server.js and lib/assets/react-source/production/react-server.js are large diffs of generated/eval'd webpack bundles. The key observable change is the removal of the old process/browser.js shim module entry in favour of the new additionalAliases-driven injection, which is the expected output of the config fix. These look correct in context.

One minor note: the generated bundle diffs use eval(...) for module bodies (webpack devtool default). This is expected for the development bundle but worth confirming the production bundle doesn't inadvertently use eval (it appears to be a single-line change, so likely fine).


Summary

The core fix (adding process to additionalAliases) is sound and addresses the reported root cause. The dependency updates are largely mechanical. The two items most worth a second look before merging are the webpack-cli/webpack-dev-server version pairing and whether css-minimizer-webpack-plugin should be bumped as part of this batch.

@justin808
Copy link
Copy Markdown
Collaborator Author

Thanks. I double-checked the two compatibility questions and the Shakapacker migration steps against the actual pinned packages on this branch.

A few concrete notes:

  • On the Shakapacker side, the relevant official upgrade docs are the v7 -> v8 guide and the v8 -> v9 guide.

    • The v8 guide's main repo-relevant migration item is the packageManager field; this repo already had "packageManager": "yarn@1.22.21" in the root package.json before this PR.
    • The v9 guide's repo-relevant config change is webpack_loader -> javascript_transpiler, plus the explicit assets_bundler setting when staying on webpack.
    • I also checked for the other v9 migration surface area and this repo does not use CSS Modules or old webpack_loader config patterns here: there are no .module.css / .module.scss imports and no import styles from ... usages to migrate.
  • For webpack-cli / webpack-dev-server: this pairing is supported with the versions in the PR.

    • webpack-dev-server@5.2.3 itself only declares a webpack peer dependency.
    • shakapacker@9.7.0 explicitly allows webpack-cli: ^4.9.2 || ^5.0.0 || ^6.0.0 and webpack-dev-server: ^4.15.2 || ^5.2.2 in its peer dependency range.
    • I also sanity-checked the local startup path with ./node_modules/.bin/webpack serve --help in test/dummy, which succeeds with the current webpack-cli@^4.9.2 / webpack-dev-server@^5.2.3 combination.
  • For css-minimizer-webpack-plugin: agreed it is older than the rest of the CSS toolchain, but it is still within its supported webpack@^5 range and the matrix is green with it as-is. I left that one untouched deliberately to keep this PR scoped to the consolidated dependency batch plus the CI regression fix, rather than widening churn with optional tool upgrades that are not required for compatibility.

  • On the generated bundles: I confirmed the production server bundle does not contain webpack dev eval(...) wrappers. The eval(...) output is limited to the development bundle, which is expected.

Given the above, I am not planning another code change on this PR for these points. If maintainers want the remaining optional toolchain bumps (webpack-cli v5+, css-minimizer-webpack-plugin refresh), I think that is better handled as a separate follow-up modernization PR instead of expanding this already-large dependency consolidation diff further.

@justin808 justin808 marked this pull request as ready for review April 3, 2026 08:44
@justin808 justin808 merged commit c71da58 into main Apr 3, 2026
65 of 66 checks passed
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.

2 participants