-
-
Notifications
You must be signed in to change notification settings - Fork 186
fix: support webpack publicPath: 'auto'
#312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: support webpack publicPath: 'auto'
#312
Conversation
|
@CharlieHelps use the PR template. resolve the conflicts with master. |
Expand this to see my work.
|
…hs\n\nTreat 'auto' as runtime-resolved publicPath so we don’t serialize it into the manifest. Adds a unit test covering the behavior.\n\nRefs #307
e530a70 to
936add7
Compare
|
@CharlieHelps compare your solution with another contributor's solution: #308 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In test/integration/location.ts, a synchronous delete (deleteSync) is awaited inside an async function, which is misleading and unnecessary. Replace with deleteAsync or drop async/await to match the chosen API.
- The new publicPath 'auto' handling looks correct and is covered by a unit test. No additional logic issues found in the modified code.
- Other changes are dependency and tooling updates aligned with newer major versions.
Additional notes (1)
- Maintainability |
src/hooks.ts:83-89
Good enhancement. To make this more robust against minor configuration drift, consider trimming whitespace before comparing against'auto'. This avoids edge cases like" auto "from custom config or stats. It also keeps behavior identical forundefinedvalues.
Summary of changes
- Added support for webpack output.publicPath: 'auto' by treating it as runtime-resolved and not prefixing manifest entries in src/hooks.ts.
- Introduced a unit test in test/unit/paths.ts to verify no manifest prefixing when publicPath is 'auto'.
- Updated tests to use del’s synchronous API (deleteSync) and adjusted CopyPlugin destination templates to new placeholders.
- Modernized ESLint config (flat config), upgraded dev dependencies (eslint 9, vitest 4, copy-webpack-plugin 13, style-loader 4, webpack-sources 3), and updated CI workflow checkout behavior.
- Minor test code cleanups (removed lint suppression comments, small casting/parenthesis tweaks).
|
All set—updated the PR body to use the repo’s template and resolved the conflicts with master by rebasing onto master at 6be4e24 (Oct 30, 2025). Force-pushed the branch so the PR now only contains the What I did
Verification# Build
$ pnpm build
# OK
# Tests (Vitest)
$ pnpm test
# 12 test files passed, 4 skipped; 45 tests passed, 5 skipped
# Lint
$ pnpm lint
# ESLint: 0 errors, 1 warning; Prettier: README.md and package.json unchangedSelf review:
|
|
Thanks—here’s the comparison between this PR (#312) and #308.
Recommendation
References
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'auto' handling is correct and covered by a unit test. A small hardening—trimming whitespace before the 'auto' comparison—would prevent subtle misconfigurations from leaking into the manifest. Optionally, add a test case for whitespace-padded 'auto' to guard against regressions. No performance or complexity concerns identified in the modified code.
Additional notes (1)
- Maintainability |
test/unit/paths.ts:75-94
The new test captures the core behavior well. For additional confidence, you could add a companion test that uses a whitespace-padded' auto 'value to ensure we don’t regress if such input appears from custom configs or stats output.
Summary of changes
- Added handling in
src/hooks.tsto treat webpackoutput.publicPath: 'auto'as runtime-resolved, preventing the literal'auto'from being prefixed into manifest entries. - Introduced a focused unit test in
test/unit/paths.tsvalidating that no prefixing occurs whenpublicPathis'auto'. - Commentary clarifies the rationale and links to webpack docs for automatic publicPath behavior.
Yes please |
Expand this to see my work.
|
…\n\n- publicPath 'auto' + basePath still prefixes manifest keys\n- publicPath 'auto' + plugin publicPath override applies to values\n\nRefs #307
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'auto' handling is correct and well-scoped, with targeted tests validating key scenarios. One maintainability improvement is to normalize and type-guard the publicPath before comparing to 'auto' to avoid edge cases like ' auto '. Optionally, add a test covering whitespace-padded 'auto' to protect against regressions. No performance or complexity concerns identified in the modified code.
Additional notes (1)
- Maintainability |
test/unit/paths.ts:75-94
The new tests cover the core'auto'behavior plus basePath and plugin override. For extra guardrails against subtle regressions, consider adding a case where webpack reports a whitespace-padded value (e.g.,' auto '). This would fail with the current implementation but pass once normalization is applied.
Summary of changes
- Added handling for webpack
output.publicPath: 'auto'insrc/hooks.tsso the manifest does not serialize the literal'auto'into asset paths. - Introduced three unit tests in
test/unit/paths.tsverifying:- No prefixing when webpack
publicPathis'auto' - BasePath still prefixes manifest keys with
'auto' - Plugin-level
publicPathoverride takes precedence when webpack uses'auto'.
- No prefixing when webpack
- Included inline documentation referencing webpack’s Automatic PublicPath docs.
|
Ported the two extra tests from #308 and credited @chouchouji in the commit message. They verify that basePath still applies when webpack’s publicPath is 'auto' and that a plugin-level publicPath override wins in that mode. See commit b0285b7.
Changes
Verification# Install (aligns ESLint to lockfile v8)
$ pnpm install
# OK
# Build (TypeScript)
$ pnpm build
# OK
# Tests (Vitest)
# Test Files: 12 passed, 4 skipped (16)
# Tests: 47 passed, 5 skipped (52)
$ pnpm test
# Lint (ESLint 8)
# 0 errors, 1 warning (preexisting: src/helpers.ts no-param-reassign)
$ pnpm lintSelf review: only maintainability notes (optional test refactors/normalization). Skipped to keep this change scoped to porting tests as requested. Re-requested review from @shellscape and @CharlieHelps. |
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Closes #307
Description
Support webpack's automatic
output.publicPath: 'auto'without serializing the literal string into the manifest. WhenpublicPathis'auto', the path is resolved at runtime by webpack; this PR ensures manifest values are not prefixed withauto. Includes a focused unit test intest/unit/paths.tscovering the behavior.Maintenance: rebased on
master(6.0.0 at 6be4e24) and resolved conflicts by dropping a redundant chore commit so the branch now cleanly reflects only the functional change and its test.Verification