Repository navigation
Migrate JS build from webpack to Vite - #13331
Conversation
…ation Fixes found by the strict code-quality review of the webpack→Vite commit: - Fix scripts/js-build-parity.sh, which recovered webpack.config.js via `git show HEAD:` even though that file was deleted by the same commit. It now resolves the most recent ref that still contains the file (walking `git log --all --follow`), overridable via WP_GIT_REF. - Extract duplicated options from vite-js.config.mjs / vite-js-iife.config.mjs into vite-js-shared.mjs (commonBuildOptions, parameterized jsBuildMarker). - Derive VITE_VERSION from the installed vite package instead of a hardcoded "keep in sync with package.json" constant. - Move the transform plugins (jquery $ injection, jquery-ui AMD deps) and the chunk-name contract into vite-js-plugins.mjs with new unit tests (tests/unit/js/vite-js-plugins.test.js); widen jest's transform regex to cover .mjs and add a moduleNameMapper entry for the plugin module. - Remove the 58 dead webpackChunkName comments left in the JS source (chunk names are driven by CHUNK_NAME_MAP, not the comments). - Fix dangling css-vite-migration.md references in the migration docs and update the plan doc's status from "plan only" to executed. - Add code-quality-review-findings.md tracking the review items. Verified: parity harness PARITY OK, jest 583/583, eslint clean, make js. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <[email protected]>
…marker The Makefile's post-build shell loop prepended the AGPLv3 LibreJS magnet comment to every JS file. Both Vite configs now emit the header/footer themselves via output.postBanner/postFooter (applied after minification, so the comments survive), with shared constants in vite-js-shared.mjs. Also drops the "built by Vite" marker entirely (and the jsBuildMarker plugin, the manual VITE_VERSION constant, and the parity harness's marker check), and uses rolldownOptions as the canonical Vite 8 option name. Verified: parity harness PARITY OK, jest 583/583, make js. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <[email protected]>
Vite 8 (rolldown) lowers syntax with Oxc and minifies JS with its native Oxc-based minifier; CSS is minified by LightningCSS. esbuild is no longer installed or used by the project — only remaining trace is vite's optional peer dependency in the lockfile. Updates comments in vite-js-shared.mjs, vite-css.config.mjs, index.js, and the migration/README docs. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <[email protected]>
The two regex-based source-mutation plugins from the Vite migration are gone,
making all jQuery usage explicit and grep-able:
- injectJqueryGlobals() deleted: an AST-based eslint `no-undef` scan found 31
source files + 4 test files that genuinely reference unbound `$`/`jQuery`
(the regexes were also injecting unused imports into 5 files via strings,
regex literals, and comments). Each now has `import $ from 'jquery';`
(index.js gets both `$` and `jQuery`), and `$`/`jQuery` were removed from
eslint globals so unbound usage is a `no-undef` error again.
- jqueryUiAmdDeps() deleted: replaced by four explicit wrapper modules
(jquery-ui-{tabs,dialog,autocomplete,sortable}.js) that list each widget's
AMD deps in topological order, preserving per-widget tree-shaking (a single
explicit module had previously been rejected for forcing a 79 KB shared
chunk onto every widget page). The 7 importing files use the wrappers, and
new unit tests pin the wrappers to the installed jquery-ui package so an
upgrade that changes the AMD graph fails loudly.
- Adds docs/ai/jquery-usage.md, an inventory of the remaining jQuery surface
(33 source files; 8 use $.ajax, 7 depend on jquery-ui widgets, 6 use
colorbox) to seed the eventual jQuery removal work.
Verified: js-build-parity PARITY OK, jest 581/581, eslint clean, make js works.
🤖 Generated with Codebuff
Co-Authored-By: Codebuff <[email protected]>
…enames (#13351) With all.js now a `type="module"` script that dynamically imports the bundle, DOMContentLoaded (and often `load`) fires before initAnalytics() and initServiceWorker() run, so their listeners register too late and never fire — no pageview ping, no service worker registration. Check document.readyState and run immediately if the event already happened. Also emit extracted CSS as `[name].[hash].css`: /static/build is served with `expires max` and these files bypass static_url()'s `?v=` buster, so unhashed names would be cached by browsers forever across deploys. Claude-Session: https://claude.ai/code/session_0181hgjqkv2tyxa8XJi3v78y
|
Here is the one issue from the automated reviews that I think is worth addressing: The unhashed all.js gets stuck in browser caches after a deploy How the new build works. Your page's footer loads all.js (with ?v=hash added by static_url). That file is now a tiny "facade": all it does is import('./main..js'), and the real app lives in main..js plus ~50 other hashed chunks. Because the chunk filenames contain a content hash, they're safe to cache forever. That's the standard trick. The wrinkle. Vite has a little helper function that lazy chunks use to load their dependencies (it's what injects the and CSS tags). Vite puts that helper inside the entry file, all.js, and every chunk that needs it does import { t } from "./all.js". Note that path: bare ./all.js, no ?v=. So on every page view the browser actually fetches all.js twice, once as all.js?v=abc from the footer, and once as bare all.js because main.js imported it. I confirmed both requests in the network log. Why that's a problem in production but not on your laptop. Prod nginx serves everything under /static/build with expires max, i.e. "cache this forever." That's fine for hashed files. But bare all.js has no hash and no ?v=, so a returning visitor's browser will keep serving the old all.js from cache after you deploy. That old file says import('./main..js') and preloads the old dep list. Those files no longer exist, so you get ~10 404s and Vite throws a vite:preloadError that nobody catches (Sentry will report it), on every page view, until the user's cache eventually clears. The app still works, because the fresh all.js?v=new already loaded the new main, but it's noisy and it's per-user forever. Dev has no nginx, so you never see it locally. What fixing it looks like. One of: (a) stop chunks importing the entry at all, e.g. build.modulePreload: false if the CSS injection still works without it, or get Vite/rolldown to put the helper in its own hashed chunk; or (b) make the bare all.js not cache forever (a nginx no-cache rule for just that file). Then verify on the testing server across two deploys. |
Ditch second default import and replace jQuery() uses with $(). Only file in codebase that imported both $ and jQuery from 'jquery'.
The module no longer contains any transform plugins (those were deleted in favor of explicit jQuery imports); it only holds CHUNK_NAME_MAP + chunkName. Rename to reflect its actual contents and update the import in vite-js.config.mjs, the eslint file list, the jest moduleNameMapper, and the test file.
…rename Remove code-quality-review-findings.md from the shipped tree (working scratch from the migration review; fold the verification status into the PR description). Also commits the two references that were missed in the vite-js-plugins.mjs -> vite-js-chunk-names.mjs rename: the test import specifier and the module's own doc comment.
vite-lit.config.mjs is the only Vite config asking for `minify: 'terser'`, and terser has never been in package.json. It resolved anyway because webpack pulled it in transitively via terser-webpack-plugin, whose ^5.31.1 range is what pinned the lockfile's terser 5.31.1. Vite only ever listed it as an optional peer. That makes it a landmine for internetarchive#13331, which drops webpack for Vite: without terser-webpack-plugin the package disappears, `make lit-components` dies with "terser not found", and because the target only swaps the bundle into place on success, the previously built ol-components.js survives. Nothing in the browser looks broken; the Lit elements are simply frozen at the last good build. On testing that stranded OlSelectPopover on a revision that never fires ol-select-popover-request-open, so the search modal's facet droppers listened for an event nothing emitted and opened silently uncounted. Switch to the default minifier, matching every other Vite config here rather than adding a dependency to keep one config's choice alive. Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf
|
Heads up: dropping
The new runtime That leaves rm -rf $(BUILD)/lit-components
mv $(BUILD)/lit-components_new $(BUILD)/lit-componentsso a failed build silently leaves the previous Two ways to fix: add |
…3064) * feat(search): add count + loading state to OlSelectPopover Extends item shape from {value, label} to {value, label, count?}. Renders count as a muted, comma-formatted trailing number when present. Adds host-driven loading boolean attribute, swaps suggestions list for a spinner while true. No fetch or domain logic added - stays a pure UI component per the building-components architecture guide. Part of #13060 * test(search): unit tests for searchFacets and mergeFacetCounts * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix(search): match facet counts by label not value * fix(search): match /search in the facets query The facet counts came from a plain WorkSearchScheme, so they ignored the solr_editions setting and skipped the edition-level block-join that /search.json opts into. Availability-filtered counts could then disagree with the result set they were meant to describe. Pass solr_editions through from the request context and request the same key,editions fields, so a facet count and the search it annotates are answering the same question. Claude-Session: https://claude.ai/code/session_018u1CiBQgEQ8YEBqBf3J5Hc * fix(dev): proxy /search/facets.json to FastAPI The endpoint lives only in FastAPI, and local dev has no reverse proxy in front of the two servers — web.py forwards an allowlist of paths to fast_web instead. /search/facets was missing from it, so the browser got a 404 from web.py on :8080 and the language dropper silently fell back to its uncounted list. The `/search` entry only matches /search.json. Claude-Session: https://claude.ai/code/session_018u1CiBQgEQ8YEBqBf3J5Hc * feat(search): context-aware language counts in the search modal The header modal's language dropper always showed the whole catalogue — only the /search filter row got counts. It now fetches the same facet counts for the query on screen. - Move mergeFacetCounts into searchFacets.js so both surfaces share it, and join on `value` (the MARC code) rather than `label`. Both sides localize their labels independently, so a label join would drop every row under some UI languages. The old fixtures used value === label, so nothing caught it. - Strip an existing filter on the field being counted. Solr ANDs an fq on the faceted field, which would zero out every other language and strand the patron on the one already ticked. - Cache the merged list per query + availability, so re-opening the dropper on an unchanged search costs nothing and ticking a checkbox doesn't refetch. - Open the dropper only once its counts are in. It used to open on a spinner, then swap in a shorter, re-sorted list — two jarring transitions under the pointer. ol-select-popover now fires a cancelable `ol-select-popover-request-open`; a host that defers calls show() when ready. Hosts that don't listen are unaffected. The wait is capped at 500ms, past which it opens on the catalogue as before. - Make the dropper's loading text translatable (`loadingLabel`) — it was the one hardcoded string left in the component. Claude-Session: https://claude.ai/code/session_018u1CiBQgEQ8YEBqBf3J5Hc * fix(search): wire context-aware facet counts to shared searchFacets.js, fix watch script missing lit-components build, fix mobile tray clipped by dvh * fix(build): rebuild the Lit bundle in npm run watch `watch` ran watch:js, watch:css and watch:components but never watch:lit-components, so editing anything under openlibrary/components/lit/ left static/build/lit-components/production/ol-components.js untouched. The page kept serving the previously built bundle, with no error to show for it. This bites hardest when a change spans both bundles: the search modal's facet counts need SearchModal.js (webpack) to hear an event that OlSelectPopover.js (the Lit bundle) fires, so a stale Lit bundle leaves the listener attached to an event nothing emits and the dropper opens silently uncounted. Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf * fix(build): stop the Lit bundle depending on an undeclared terser vite-lit.config.mjs is the only Vite config asking for `minify: 'terser'`, and terser has never been in package.json. It resolved anyway because webpack pulled it in transitively via terser-webpack-plugin, whose ^5.31.1 range is what pinned the lockfile's terser 5.31.1. Vite only ever listed it as an optional peer. That makes it a landmine for #13331, which drops webpack for Vite: without terser-webpack-plugin the package disappears, `make lit-components` dies with "terser not found", and because the target only swaps the bundle into place on success, the previously built ol-components.js survives. Nothing in the browser looks broken; the Lit elements are simply frozen at the last good build. On testing that stranded OlSelectPopover on a revision that never fires ol-select-popover-request-open, so the search modal's facet droppers listened for an event nothing emitted and opened silently uncounted. Switch to the default minifier, matching every other Vite config here rather than adding a dependency to keep one config's choice alive. Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf * fix(search): route facet counts through /_fast on testing /search/facets.json only exists in FastAPI. Local dev proxies the bare path through web.py, but everywhere else web.py answers it from the deprecated- endpoint handler, which raises internalerror unless LOCAL_DEV is set. On the testing server that is a hard 500: DEPRECATED ENDPOINT ACCESSED: /search/facets.json. This endpoint has been migrated to FastAPI and should not be accessed in production. fetchFacetCounts() surfaces that as a failed load, so the dropper silently falls back to its uncounted catalogue. Testing exposes FastAPI behind /_fast, where the same query returns counts, so pick the prefix from the hostname the way testingStatusUrl() already does for /status/testing.json. Claude-Session: https://claude.ai/code/session_011AqgrMKn29JVncut49JNtf --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Lokesh Dhakar <[email protected]>
Resolves package-lock.json conflict by regenerating against the merged package.json (keeps master's concurrently v10 bump alongside the Vite dependency changes). Also picks up #13064's terser fix (minify: true).
Delete docs/ai/js-vite-migration-plan.md and js-vite-migration-progress.md, which were committed to track work during the PR but aren't meant to ship. Drop their dangling references from the Vite configs, parity harness, and AI docs to keep everything pointing at files that exist.
|
@lokesh thanks for the feedback/fix this is helpful. I'm going to try to get the jenkins deploy fixed so we don't have || true. Where did you see that? |
| // jquery plugins to provide author, language, and subject autocompletes. | ||
| import 'jquery-ui/ui/widget'; | ||
| import 'jquery-ui/ui/widgets/autocomplete'; | ||
| import 'jquery-ui/ui/widgets/mouse'; |
There was a problem hiding this comment.
I think dropping mouse might make it stop working on touch screens ; we should test. Specifically reordering (1) covers in the manage covers window, and (2) authors in the work edit page.
EDIT: Ah it seems like the new helper file is still importing these, so not a concern. It seems like this is the only spot that uses that helper file, so maybe let's inline it directly...? Although I see there's a bit of a pattern going with having the larger jquery ui components wrapped together in a single file, so that makes sense. Never mind, leave as is!
There was a problem hiding this comment.
Bookmark ; part way through the cr!
…entary Clarify that page-JS syntax is lowered to the build.target in vite-js-shared.mjs (['safari11.1','ios11.3'], matching browserslist) rather than the misleading "~Chrome 61" phrasing, now that all.js ships as a type=module script. Trims the redundant Chrome/Edge/Firefox note from the config comment. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <[email protected]>
The JS build sets base=/static/build/js/, and Vite prepends base to root-absolute url(/static/...) references in CSS it treats as public files, producing /static/build/js/static/... 404s. This made the carousel arrow icons disappear. Add an experimental.renderBuiltUrl hook that returns public urls unchanged, and a parity check so the regression is caught in js-build-parity.sh. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <[email protected]>
|
So, turns out long term we should probably avoid static urls for assets. That's not really the way to do things with vite. However, that should be a followup because it requires changing a lot of file. Here's the AI instructionsFollow-up: remove the root-absolute url hack in the Vite JS buildThe short versionThis PR added a small hack to fix the carousel arrows (and other icons) that What went wrongThe JS build writes its output to Vite joined the The word "static" appears twice. That path does not exist, so the browser What this PR didWe added the This restores the behavior webpack had with Why this is a stopgap
The follow-up workConvert the root-absolute urls in CSS to relative paths, for example: /* before */
background: url(/static/images/icons/icon_x.png);
/* after */
background: url(../../images/icons/icon_x.png);Then Vite fingerprints the file ( When the last root-absolute url is gone, delete all of this together:
ScopeThere are 29 CSS files with 46 unique
Removing slick later will not remove the need for this hack — the second Templates keep using root-absolute urls via How to verify the follow-up
|
…nts, link CSS-url follow-up Second pass of the thermo-nuclear review of the Vite JS build migration: - Remove "*.config.mjs" from the eslint global ignores. The files-list re-inclusion for vite-js.config.mjs / vite-js-iife.config.mjs was a no-op against that pattern (flat-config ignores win), so the two new configs were silently unlinted. Add vite-asset-urls.mjs and custom-elements-manifest.config.mjs to the ESM config block so every build config gets node globals + no-console: off. - Delete the three webpackChunkName magic comments that came back via the master merge (index.js subjects imports, subjects.js graphs import) and add a no-warning-comments eslint guard keyed on the term so the cleanup stays enforced. - Point the renderBuiltUrl stopgap follow-up at the PR discussion comment instead of the untracked drafts/issue-remove-root-absolute- css-urls.md file. Verified: eslint . clean, jest 735/735, make js builds. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <[email protected]>
| "watch": "npx concurrently --names js,css,components,lit --prefix-colors --kill-others npm:watch:js npm:watch:css npm:watch:components npm:watch:lit-components", | ||
| "watch-polling": "env FORCE_POLLING=true npm run watch", | ||
| "watch:js": "env NODE_ENV=development npx webpack --watch --progress", | ||
| "watch:js": "npx concurrently -n all,sw,partnerLib --kill-others 'npx vite build -c vite-js.config.mjs --watch --mode development' 'IIFE_ENTRY=sw npx vite build -c vite-js-iife.config.mjs --watch --mode development' 'IIFE_ENTRY=partnerLib npx vite build -c vite-js-iife.config.mjs --watch --mode development'", |
There was a problem hiding this comment.
The cost of this line is that I'll have to ask AI what it does every time I want to feel confident about the answer, but I still think it's the right approach rather than splitting up the watchers (even though service-worker.js and partner_ol_lib.js are rarely touched).
Co-authored-by: Lokesh Dhakar <[email protected]>
cdrini
left a comment
There was a problem hiding this comment.
Lgtm ; a few things stuck in a sort of in-between land it would be good to clean up in future PRs!
Namely:
- Can we try switching to vite's chunking/bundling and remove the chunk map index? That index feels like it will almost immediately start falling out of date
- Can we remove or combine some of the build files?
- Do we need a separate file for IIFE?
- Adding the eslint plugin for ensuring we have browser polyfills
| * after the imported file. `bundlesize.config.json` globs and deploy tooling | ||
| * are keyed on the webpack names, so remap them here. | ||
| */ | ||
| export const CHUNK_NAME_MAP = { |
There was a problem hiding this comment.
This manual list here feels a bit untenable to maintain; the webpack approach with the comments was maintainable, but vite is more opinionated so I think we would need to switch to using vite's own non-manual/configurable chunking strategy. It would useful if we could see what that future looks like to see if it causes any perf issues or if it we can switch to it without issue.
| * Tracked in the follow-up discussion on the migration PR: | ||
| * https://github.com/internetarchive/openlibrary/pull/13331#issuecomment-5514656942 | ||
| */ | ||
| export function renderBuiltAssetUrl(filename, { type }) { |
There was a problem hiding this comment.
Can we move this helper method to the vite-shared file?
…k shim
Rolldown names chunks after the imported file, so the webpackChunkName
compat layer is no longer needed:
- Rename index.js -> main.js: rolldown names index.js chunks after the
parent directory ("js"); the rename makes it derive "main" naturally,
keeping the bundlesize main.*.js budget glob meaningful.
- Delete vite-js-chunk-names.mjs (the 16-entry CHUNK_NAME_MAP and the
"main" special case) and wire chunkFileNames as plain '[name].[hash].js'
(dot separator preserves the webpack-era filename convention the
bundlesize globs are written against).
- Repoint the four affected bundlesize globs to the natural chunk names:
user-website -> edit, modal-links -> modals, readinglog-stats ->
readinglog_stats, goodreads-import -> goodreads_import.
- Keep the jquery-ui wrapper tests as tests/unit/js/jquery-ui-wrappers.test.js;
drop the chunkName suite that only pinned the removed shim.
- Delete scripts/js-build-parity.sh (migration-only webpack parity harness).
- Drop the jest moduleNameMapper entry and eslint file-list entry for the
removed module.
Git rename detection carries master's hamburger-drawer wiring (#12326) onto the renamed main.js. Strip the webpackChunkName magic comment from the merged-in import: under Vite, chunks are named after their imported file, so the chunk is emitted as hamburger-drawer.<hash>.js naturally.
Update five stale references to index.js (docs, template/macro comments, CSS comments) to main.js. References to other, real index.js files (ile, design-system, lit components, banner, editions-table) are untouched.
The Vite preload helper is hoisted into the entry all.js, so hashed chunks import it by bare path (no ?v= checksum). Its content changes on every deploy (it embeds the current chunk hashes), so under `expires max` returning visitors hold a stale copy that preloads deleted chunks -> 404s and vite:preloadError. Serve it via an exact-match location with `expires epoch` (revalidate every request); everything else under /static/build keeps expires max.
Reword comments and docs that compared Vite settings to their old webpack equivalents — webpack.config.js no longer exists, so the comparisons read as archaeology. Settings now describe the behavior they produce. The eslint guard comment for webpackChunkName stays: that rule exists to catch webpack-era leftovers. Also update the stack line in AGENTS.md / copilot-instructions.md to name Vite instead of webpack.
|
Pushed up some changes for docs and was able to cleanup that chunk map index we didn't like. Looking good now! |
Summary
Replaces the webpack JS build with Vite (Rolldown). Same source in
openlibrary/plugins/openlibrary/js/, same features, same CI-enforced bundle-size budgets — but the build drops ~3,400 lines net and the webpack config, plugin shims, andwebpackChunkNamemagic comments are gone.What changes
all.jsis now a tiny unhashed ES-module entry (<script type="module">, loaded insite/footer.html). The app itself is a content-hashedmain.<hash>.jschunk plus ~80 lazy chunks named after their source files.webpackChunkNamecomments are removed and eslint now errors if any reappear.all.jsis a side-effect-free facade (main-entry.js) that dynamically importsmain.js, so lazy chunks never import the entry by a second URL (avoids double module instances / repeated side effects).browserslistfloor (Safari 11.1 / iOS 11.3); a short, explicitcore-jsimport list replaces babel'suseBuiltInspolyfill injection.jquery-ui-tabs/dialog/autocomplete/sortable), with unit tests pinning their dependency order./static/build/js/all.jsgets an exact-match location withexpires epoch(revalidate every request); everything else under/static/buildkeepsexpires max— see deployment note.make js/npm run watchwork as before;bundlesizebudgets kept, with globs matching the new (natural) chunk filenames.webpack.config.js,vue.config.js, and all webpack-era plugin machinery.Deployment note:
all.jscachingall.jsis unhashed and is imported by hashed chunks via its bare path (Vite's preload helper lives in the entry), so browsers fetch it without the?v=checksum the<script>tag uses. Its content changes on every deploy — it embeds the current chunk hashes — while it must stay at a stable path.web_nginx.conftherefore serves it withexpires epoch(revalidate every request) via an exact-match location; the file is ~2KB, so a 304 is cheap. Without this rule, returning visitors would hold a staleall.jsthat preloads deleted chunks, producing 404s andvite:preloadErroron page load.Testing
jest(46 suites) andbundlesize(24 checks) pass in CI;make jsbuilds cleanly.