Skip to content

fix: assorted bug fixes (close #588)#608

Open
andriytyurnikov wants to merge 4 commits intoElMassimo:mainfrom
rubakas:fix/independent-bugs
Open

fix: assorted bug fixes (close #588)#608
andriytyurnikov wants to merge 4 commits intoElMassimo:mainfrom
rubakas:fix/independent-bugs

Conversation

@andriytyurnikov
Copy link
Copy Markdown

First slice of #607, per your request to break it up. Four independent bug fixes, each in its own commit. No version bumps, no CI changes.

Fixes

vite-plugin-ruby: stop forcing sourcemap=true in production (closes #588)

The plugin was setting build.sourcemap = !isLocal, which inverted Vite's safe default:

  • production / staging → sourcemap=true (publishes .js.map publicly, exposing source)
  • development / test → sourcemap=false (breaks debugging where you'd actually want it)

Drop the override entirely. Vite's default (false) now applies in every mode; consumers opt in via their own build.sourcemap config when they want sourcemaps (e.g. uploaded to Sentry).

Tests updated:

  • vite-plugin-ruby/tests/index.spec.ts — assert sourcemap is no longer set
  • vite-plugin-rails/tests/build.spec.ts — drop .map entries from expected file list

examples/rails: gate engine alias on env var

The example used a TypeScript non-null assertion on process.env.ADMINISTRATOR_ASSETS_PATH, which is unset at build time when no Rails engine is wired up — leaving [undefined] in server.fs.allow and an alias resolving to "undefined/...". Vite ≤7 silently tolerated this; Vite 8 strictly path-resolves server.fs.allow and crashes.

Make both the alias and the fs.allow entry conditional on the env var actually being present.

vite-plugin-ruby: return after res.end() in dev /index.html 404 fallback

Without the explicit return, control falls through to next() after the response has already been ended, allowing downstream middleware to write to a closed response.

vite_ruby: guard dev_server_running? cache with a Mutex

The prior implementation read @running and wrote @running_checked_at without synchronization, so a cold concurrent call from a multi-threaded server (e.g. Puma) could read @running after it was set but before @running_checked_at was, and the next call's Time.now - @running_checked_at could raise NoMethodError on nil.

Wrap the check-or-refresh in @running_mutex.synchronize so the cache read and write are atomic.

Test plan

  • pnpm -C vite-plugin-ruby test --run passes
  • pnpm -C vite-plugin-rails test --run passes
  • Local examples/rails vite build succeeds with and without ADMINISTRATOR_ASSETS_PATH set
  • CI matrix passes

ElMassimo#588)

The plugin was setting `build.sourcemap = !isLocal`, which inverted Vite's
safe default:
  - production / staging → sourcemap=true   (publishes .js.map publicly,
                                              exposing application source code)
  - development / test   → sourcemap=false  (breaks debugging where you'd
                                              actually want sourcemaps)

Drop the override entirely. Vite's default (`false`) now applies in every
mode, and consumers opt in to sourcemaps via `build.sourcemap` in their
own vite.config when they actually want them — for production via
external upload to e.g. Sentry, or for development debugging.

The unit test in vite-plugin-ruby/tests/index.spec.ts had been encoding
the inverted behavior; updated to assert `sourcemap` is no longer set by
the plugin. The integration test in vite-plugin-rails drops the `.map`
entries from the expected file list since the example build no longer
emits them.
The example used a TypeScript non-null assertion on
process.env.ADMINISTRATOR_ASSETS_PATH that is unset at build time when
no Rails engine is wired up, leaving [undefined] in server.fs.allow
and an alias resolving to "undefined/...". Vite ≤7 silently tolerated
those undefined entries; Vite 8 strictly path-resolves server.fs.allow
and crashes with "paths[1] argument must be of type string".

Make both the alias and the fs.allow entry conditional on the env var
actually being present, so the build works whether or not the engine
fixture is wired up.
…fallback

Without the explicit return, control falls through to next() after the
response has already been ended, allowing downstream middleware to write
to a closed response.
The prior implementation read `@running` and wrote `@running_checked_at`
without synchronization, so a cold concurrent call from a multi-threaded
server (e.g. Puma) could read `@running` after it was set but before
`@running_checked_at` was, and the subsequent `Time.now - @running_checked_at`
on the next call could raise NoMethodError on nil.

Wrap the check-or-refresh in `@running_mutex.synchronize` so the cache
read and write are atomic.
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.

Sourcemaps enabled in production by default (security issue)

1 participant