Conversation
Contributor
|
@xantorres This is the right place for the CORS note. LGTM. Optional nit: mention that the script request can still return 200 while the browser blocks execution (the page may look populated but is not interactive). |
Author
|
Added in 48a960a: both CORS paragraphs now state that the script request still returns 200 and server-rendered content still appears, so the page can look populated while nothing is interactive. Thanks for the review. |
This was referenced Aug 22, 2026
robinv8
pushed a commit
to apache/answer
that referenced
this pull request
Sep 18, 2026
Fixes #1580. Part of #1578. Step 2 of 3, atomic. Depends on #1582 (the Go asset contract, now merged into `dev`), which is why the server-side parsing does not appear in this diff. Replaces `react-scripts` and `react-app-rewired` with Vite. The configuration preserves the output contracts the server depends on: the build directory stays at `ui/build`, which `ui/static.go` embeds, and emitted assets stay under `static/`, which `internal/router/ui.go` serves as a route. Files keep the `static/js`, `static/css` and `static/media` grouping, which the `analyze` script matches on; the ignore rules for `ui/build`, previously pinned to that layout's directory depth, now ignore everything under `build` except the tracked favicon. Production sourcemaps stay on, and the `REACT_APP_` prefix is retained so `ui/scripts/env.js` remains the single source of truth for configuration shared with the server. That same script's `public_url` value now also drives Vite's `base` configuration, and the manifest link in `ui/index.html` uses Vite's `%BASE_URL%` macro instead of a hardcoded path, so a non-root deployment keeps every asset and manifest reference prefixed the way it did under the previous toolchain's `PUBLIC_URL` wiring. Agentic tooling did the mechanical work in this series; every change was reviewed by a human before being committed. ### The one server-side line `header.html` re-emitted the paths `GetStyle()` finds as classic scripts. An ES module loaded through a classic script tag fails on its first import, so the tag is now declared as a module and carries `crossorigin`, matching the tag Vite's own build emits for the client-rendered entry point. Rewriting the built `index.html` instead was not an option, because `internal/router/ui.go` serves that same file to boot the SPA and it cannot misdeclare its own script type. A `type=module` script fetches in CORS mode, so any CDN origin serving these files needs to send the matching CORS headers, with or without `crossorigin` present; default same-origin deployments are unaffected. The note for CDN users is in the CDN plugin READMEs, apache/answer-plugins#326, per the placement decided on #1567, and the release notes for the version that ships this should carry a short pointer to it. ### Route code splitting Routes loaded pages with `` lazy(() => import(`@/pages/${pagePath}`)) ``. That shape cannot be statically analyzed, so no page received its own chunk and the specifier reached the browser untransformed, leaving every lazily routed page unable to load. Pages are now enumerated with a bounded glob covering the three directory depths routes actually use, excluding component subtrees so their own index files do not become route chunks. A page path with no matching module now rejects with the requested path and the list of known keys, surfacing through the existing route error boundary. ### Behaviour changes, called out deliberately **The custom stylesheet link now derives its href from the build's own base, not a separately computed value.** `PageTags` built the `/custom.css` link from `process.env.PUBLIC_URL`, which the previous toolchain exposed with its trailing slash already stripped; at the default configuration that value was an empty string, resolving to `/custom.css`. `import.meta.env.BASE_URL`, the direct equivalent under the new toolchain, keeps the trailing slash, so substituting it in the same place would resolve the same default configuration to `//custom.css` instead. The trailing slash is stripped explicitly before the substitution, reproducing the previous output at the default configuration and staying correct away from it. The output here is unchanged; only the mechanism it depends on is. **Two routes were dead ends and now fail loudly instead of silently.** `pages/403` and `pages/Admin/UserOverview` both referenced modules that did not exist in the tree, and both failed the same way under the previous toolchain, silently rendering blank. With the glob above, both now report the missing path through the route error boundary. Step 3 repoints `pages/403` at `pages/404/403`, an existing component, so that route renders; `pages/Admin/UserOverview` stays a visible gap because creating the missing page is a content decision. Neither is a regression. **The markdown editor now renders its themed background.** `src/components/Editor/index.scss` read `var(-bs-body-bg)` with a single leading dash. That is not a valid custom property reference, so the declaration was discarded and the editor never received the background it asks for. The previous CSS minifier accepted the invalid value; the current one rejects it outright and fails the build (`[lightningcss minify] Unexpected token Ident("-bs-body-bg")`), which is how it surfaced and why the one-line fix is part of this PR rather than a follow-up. Fixing it changes rendering. ### Dependency removals `react-scripts`, `react-app-rewired`, `customize-cra` and `config-overrides.js` are gone, and `yaml-loader` is replaced by the equivalent Vite plugin, pinned to the schema the previous loader used so bare dates and merge keys keep parsing the same way they did before. The scaffold test the old toolchain generated (`App.test.tsx`) and its jest packages go with it; this project has no test runner and no unit tests, before or after. Three removed packages were already inert before this migration. Both purgecss packages were declared but wired nowhere: no postcss config exists, the overrides file never referenced them, and no script invoked them. `buffer` was aliased and provided as a global, but no application source uses it, and the one dependency requiring it declares `buffer: false` in its own browser field. `sass` and `@types/node` are raised to the versions the toolchain requires; the previous `sass` predates the async compiler API it now calls. `sass` is pinned below the release that begins deprecating `@import`, which this project uses across 30 files. Migrating those to `@use` is a separate concern. Bootstrap's own Sass internals print dozens of dependency deprecation warnings on every build, unrelated to anything in this project's own styles; `vite.config.mts` sets `css.preprocessorOptions.scss.quietDeps: true` to silence those specifically while still surfacing warnings from this project's own stylesheets. One such app-own warning remains in this PR's build output, a mixed-declarations notice from `Comment/index.scss`; the one-line reorder that clears it is a step 3 nit. The eslint config no longer extends `react-app/jest`, which shipped inside `react-scripts` and configured rules for a test suite this project does not have. ### A failure found only by running the built application With the build green and the dev server working, the built application did not boot. React never mounted, the page showed its loading spinner indefinitely, and the browser console was empty. `i18next` attaches its resource-store methods to the instance inside `init()`. The builtin plugins register their translations while their modules are being evaluated. Whether that happens before or after `init` depends on how the bundler groups and orders chunks, so the previously working order was incidental rather than guaranteed. When it inverts, the registration throws while the entry module is still evaluating, which takes the application down before it mounts and produces no console output. Registration now happens immediately only when there is an initialised instance to register into, and otherwise falls to the `initialized` handler the code already installed, which is correct in either order. The check that guards both orders is part of step 3. ### Parity fixes found in review **Bootstrap icon fonts.** The bootstrap-icons stylesheet points at font files under a path the bundler could not resolve on the first migration pass, so the build silently emitted zero font files and every icon rendered as a missing-glyph box. The stylesheet now overrides the package's font-directory variable to a path Vite can resolve; the fonts are emitted, and the boot test below confirms they are served. **Type checking.** `pnpm build` now also runs `tsc --noEmit` before the bundler runs, and is clean today. The previous toolchain ran its checker in the dev server (blocking) and during builds (downgraded to warnings by this project's `TSC_COMPILE_ON_ERROR` setting); none of that carried over when the bundler changed, so until this fix a type error shipped with no signal at all. **Yaml parsing.** The Vite yaml plugin defaults to js-yaml's more permissive schema, which resolves bare dates to JS `Date` objects and enables merge keys; the previous loader's schema kept both as plain strings. The plugin is now pinned to that same schema. **Declared Node range.** The bundler's declared support range is `^20.19.0 || >=22.12.0`. `ui/package.json`'s own `engines.node` allowed `>=20`, which admits versions below that floor, and now matches it exactly. The release workflow's pinned Node, which also sat below the floor, is raised to `20.19.0` to match. That version bump is the only change this PR makes anywhere under `.github/`; it does not add a job. **Typed environment variables.** `import.meta.env.REACT_APP_*` previously typed as `any`, since no `ImportMetaEnv` augmentation existed; a typo'd key would compile clean and only surface as a missing value at runtime. The two keys the app reads this way, `REACT_APP_API_URL` and `REACT_APP_BASE_URL`, are now declared, with `strictImportMetaEnv` enabled so an undeclared key is a type error instead of a silent `any`. ### Verification On `dev` at `ace02c49`, which includes step 1, plus these commits: `pnpm install --frozen-lockfile` under the pinned `pnpm@9.7.0` and `pnpm build` (which now includes `tsc --noEmit`) complete; the build prints three warnings, each once: `front-matter`, a dependency unrelated to this project's own pinned `js-yaml@^4.1.0`, pulls in a legacy `js-yaml@3.x` copy whose `buffer` import gets externalized for browser compatibility; the Sass mixed-declarations notice from `Comment/index.scss` mentioned above; and one chunk over the default size threshold, see the note on chunking below. `go build ./...` and `go vet ./...` pass; `TestGetStyleResolvesBuiltAssets` passes against the Vite output; a search for `react-scripts`, `react-app-rewired`, `customize-cra` and `config-overrides` finds nothing outside the lockfile. The built binary was booted against sqlite3 and loaded in a browser: `/` renders with the client mounted (React's fiber container present on the root element), the module entry script and both entry stylesheets are fetched, and the console shows no errors. The same holds for `/tags`. The server-rendered `/` response itself carries the module entry script and both entry stylesheets, which is `GetStyle()` from step 1 finding them in the Vite output and `header.html` re-emitting them. The three `make check-ui` guards from #1567 (asset paths, non-default locale, plugin i18n order) are not in this PR; they arrive as step 3 and were run green against the equivalent tree there. ### Measurements Captured for #1567 at its head `d06b623`, against `main` at `3b9f137`, same machine, same Node and package manager versions, clean tree and clean install on both sides, five runs per timing metric. They are carried over rather than recaptured: the frontend build inputs here are the same as at that head, minus the step 3 nits and plus the newer locale files on `dev`. | Metric | Before | After | |---|---|---| | Cold production build | 19.87s | 4.77s | | Warm build | 8.75s | 4.87s | | Dev server time to ready | 7104ms | 359ms | | HMR latency | 421ms | 139ms | | Bundle JS, raw / gzip | 3477.60KB / 1198.75KB | 3010.49KB / 1035.57KB | | Bundle total, raw / gzip | 4290.39KB / 1652.24KB | 3763.97KB / 1474.34KB | | Direct dependencies | 75 | 65 | | Packages installed | 1578 | 689 | | Audit findings, critical/high/moderate/low | 4/78/63/13 | 1/50/38/6 | Five things worth stating rather than leaving to be inferred: - Cold and warm builds are within noise of each other, because the new build has no meaningful persistent cache to warm and now also runs a type check on every build, cold or warm alike. The old toolchain had a real warm cache, which is why its two figures differ so much. The comparison to draw is cold against cold. - Dev server time-to-ready is wall clock on both sides, including process spawn. The new tool self-reports a much smaller number that excludes that, and using it would compare two different quantities. - Chunk boundaries differ structurally between the two bundlers, so the bundle rows compare total shipped bytes rather than like-for-like chunks. - HMR latency was measured once, at commit 7200ca3 on the #1567 branch, on both toolchains, and carried forward from there: none of the commits since touch the hot-update path. - The bundle totals include the bootstrap-icons font files the first migration pass had silently dropped, and the cold build includes the restored type check. Both are named costs of parity fixes, already folded into the deltas. ### Not included This PR adds no CI job for the frontend. The project runs no frontend job today, and a build on every push is a cost a maintainer should choose to take on, not one this migration should impose. One constraint carries forward for whoever wires that job later: a bare `go test ./...` reports ok while asserting nothing about the built asset paths, because `TestGetStyleResolvesBuiltAssets` skips when no frontend build is embedded. Any future CI job needs to build the frontend first. The dev server now binds to loopback only by default; reaching it from another device on the network needs an explicit `--host` flag. Create React App's SVG-as-component imports (`import { ReactComponent as X } from './x.svg'`) are not carried over to this configuration. Nothing in this codebase used them. The commits carry `(cherry picked from commit ...)` lines pointing at the branch behind #1567, where these changes were first reviewed and where the regression matrix (subdirectory deploy, OAuth callbacks, absolute CDN `public_url`) was run.
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
2 participants
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.
Adds a Notes/CORS section to the cdn-aliyun and cdn-s3 READMEs: once the frontend loads as ES modules (apache/answer#1567), a CDN origin must send Access-Control-Allow-Origin for the site origin or the browser refuses to run the entry script and pages load without JavaScript. Each section carries a minimal provider-native CORS rule (OSS XML for Aliyun, S3 JSON for AWS) plus a CloudFront note covering origin-request and response-headers policies. Section shape follows the existing Notes convention in storage-s3's README. Requested by review on apache/answer#1567; companion to it, best merged with or after.