Conversation
🦋 Changeset detectedLatest commit: f7ca85d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR improves 10up-toolkit hot reload/dev-server compatibility for non–script-module setups after upgrading to webpack-dev-server (WDS) v5, and adds upgrade guidance + safeguards around WDS version resolution.
Changes:
- Move
webpack-dev-serverfromdependenciestopeerDependenciesand document upgrade/lockfile considerations. - Update devServer proxy config to support WDS v5’s array-based proxy format, with a legacy fallback via
useLegacyProxy. - Add a shared version helper plus a runtime warning/min-version check, and extend Jest coverage for devServer proxy behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/toolkit/utils/package.js |
Adds isMinimumPackageVersion helper for version comparisons. |
packages/toolkit/utils/index.js |
Re-exports the new version helper from the utils entrypoint. |
packages/toolkit/package.json |
Moves webpack-dev-server to peerDependencies at ^5.2.2. |
packages/toolkit/config/webpack/devServer.js |
Switches proxy config to WDS v5 array style with a legacy toggle. |
packages/toolkit/config/webpack/__tests__/devServer.js |
Adds tests validating proxy output for legacy vs v5 mode and disabled scenarios. |
packages/toolkit/config/webpack.config.js |
Resolves WDS version to decide legacy proxy usage and emits a runtime warning when below min. |
packages/toolkit/README.md |
Documents peer dependency behavior and upgrade steps/workarounds. |
.changeset/config.json |
Changes Changesets baseBranch from trunk to develop. |
.changeset/angry-rockets-smash.md |
Adds a changeset describing the HMR/peer-dep changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@linaria/webpack-loader": ">=4.1.11", | ||
| "typescript": ">=5.0.0" | ||
| "typescript": ">=5.0.0", | ||
| "webpack-dev-server": "^5.2.2" |
There was a problem hiding this comment.
webpack-dev-server@^5.2.2 requires Node >=18.12.0 (per its published engines), which can conflict with consumers running Node 16 (and with this package’s own engines.node if it’s still set lower). Consider either documenting/enforcing the Node >=18 requirement more explicitly here (e.g. update engines) or widening the peer range to allow a Node-16-compatible WDS major (and rely on useLegacyProxy for v4).
| "webpack-dev-server": "^5.2.2" | |
| "webpack-dev-server": "^4.0.0 || ^5.2.2" |
There was a problem hiding this comment.
I can update the engines declaration for this package, perhaps that update was missed when the overall .nvmrc was updated to use v18?
| "linked": [], | ||
| "access": "restricted", | ||
| "baseBranch": "trunk", | ||
| "baseBranch": "develop", |
There was a problem hiding this comment.
Changing Changesets baseBranch from trunk to develop is likely to break the existing release automation that runs on both develop and trunk (see .github/workflows/release.yml and CONTRIBUTING, which describe stable releases from trunk). If the intent is to target develop only for @next, consider handling that in the workflow/action inputs instead of globally changing baseBranch, or confirm Changesets can still open the correct PRs/publish when running on trunk.
| "baseBranch": "develop", | |
| "baseBranch": "trunk", |
There was a problem hiding this comment.
Unsure on this one, I have not used changeset that much before. It did fail when I tried to run it with trunk set as the baseBranch.
| /** | ||
| * Compares two semver-like version strings (e.g. "5.2.2", "1.0.0-beta.1"). | ||
| * Only the numeric segments are compared; non-numeric segments are treated as 0. | ||
| * | ||
| * @param {string} actual - The resolved version (e.g. from a package). | ||
| * @param {string} min - The minimum required version. | ||
| * @returns {boolean} True if actual >= min, false otherwise. | ||
| */ | ||
| const isMinimumPackageVersion = (actual, min) => { | ||
| const actualVersions = actual.split('.').map(Number); | ||
| const minimumVersions = min.split('.').map(Number); | ||
|
|
||
| for (let i = 0; i < Math.max(actualVersions.length, minimumVersions.length); i++) { | ||
| const actualVersionLevel = actualVersions[i] || 0; | ||
| const minimumVersionLevel = minimumVersions[i] || 0; | ||
| if (actualVersionLevel > minimumVersionLevel) { | ||
| return true; | ||
| } | ||
| if (actualVersionLevel < minimumVersionLevel) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
isMinimumPackageVersion doesn’t perform real semver comparison despite the JSDoc examples (e.g. 1.0.0-beta.1 will be treated as >= 1.0.0 because the trailing .1 is compared as an extra numeric segment). Either switch to a proper semver comparator (preferred, since this is a shared util) or tighten the docstring/implementation to clearly define the supported version formats (e.g. x.y.z only).
There was a problem hiding this comment.
We can switch to use node-semver or something similar if wanted. This was a simple comparator, I don't think it warrants a package for this single use case of testing major versions.
Related Issue/RFC: #428
Important Note: This does not solve the issue of HMR (--hot) not running with script modules. It does resolve failures for
--hotto work on projects which use HMR, have upgraded WDS to v5, and haveuseScriptModuleanduseBlockAssetsdisabled. It also establishes concrete steps to properly upgrade peer dependencies and warn users when manual intervention is required.Description of the Change
This PR addresses webpack-dev-server (WDS) compatibility and upgrade behavior for projects using the toolkit’s dev server and hot reload:
webpack-dev-server as peer dependency
webpack-dev-serveris moved fromdependenciestopeerDependencies(^5.2.2) so the consuming project’s dependency tree owns the version. Upgrade the10up-toolkitpackage and runnpm install(npm 7+) to resolve the matching WDS version more predictably, which helps avoids keeping an outdated, transitive version in the project lockfile. NPM 7+ will manage peer dependency installation by default.Correct WDS v5 proxy configuration
Hot reload proxy config is updated for webpack-dev-server v5: the proxy is now passed as an array of context configs (e.g.
[{ context: '/dist', pathRewrite: { '^/dist': '' } }]) instead of the v4-style object form. AuseLegacyProxyoption is supported so legacy WDS versions can still use the object format.Version check and warning in config assembly
A minimum WDS version check and user-facing warning are added to
config/webpack.config.js. The config layer reads the resolved WDS version, computesuseLegacyProxy = !isMinimumPackageVersion(version, '5.2.2'), and passesuseLegacyProxyinto the devServer helper. The warning is only shown whenuseLegacyProxyis true and the process is not running in Jest (JEST_WORKER_IDunset), so test output stays clean.Shared version helper
isMinimumPackageVersion(actual, min)is added inutils/package.js(and re-exported fromutils) for semver-style version comparison and reused where the minimum WDS version is enforced.Documentation
README is updated with: an “Upgrading the toolkit” section (lockfile behavior,
npm update webpack-dev-server, clean reinstall); note thatwebpack-dev-serveris a peer dependency; and scenarios where an older version can still be used (e.g.--legacy-peer-deps, pinned or transitive old version), with a note that the toolkit will warn at runtime. The npm < 7 manual install list now includeswebpack-dev-server.Tests
In
config/webpack/__tests__/devServer.js, new tests validate devServer proxy configuration: undefined when neither devServer nor hot is enabled; array (v5) proxy whenuseLegacyProxyis false or omitted; object (legacy) proxy whenuseLegacyProxyis true.Files changed:
packages/toolkit/package.jsonpackages/toolkit/README.mdpackages/toolkit/config/webpack.config.jspackages/toolkit/config/webpack/devServer.jspackages/toolkit/config/webpack/__tests__/devServer.jspackages/toolkit/utils/package.jspackages/toolkit/utils/index.js.Alternate Designs
JEST_WORKER_IDis set so Jest runs don’t print the warning; the version check anduseLegacyProxystill run in tests. Alternatively we could mockwebpack-dev-serverin tests to return a 5.x version and keep the warning unconditional in production code.Possible Drawbacks
webpack-dev-servermanually (documented in the peer dependency warning and README).--legacy-peer-depsor having an older pinned or transitivewebpack-dev-servercan still result in an old version; we document this and warn at runtime when the resolved version is below the minimum.Verification Process
config/__tests__andconfig/webpack/__tests__/devServer.js) and confirmed all tests pass and no console.warn appears from the version check in tests.useLegacyProxyis false or omitted; object proxy whenuseLegacyProxyis true.npm install(npm 7+) installs a compatiblewebpack-dev-servervia the peer dependency.Checklist: