fix: replace linux-distro with direct /etc/os-release parsing (#2236)#2308
fix: replace linux-distro with direct /etc/os-release parsing (#2236)#2308caddydove wants to merge 1 commit into
Conversation
requilence
left a comment
There was a problem hiding this comment.
Thanks for chasing down the Linux login crash — the crash guard is definitely worth landing. But the diagnosis and the credited mechanism aren't right, and I'd like the framing (and ideally the approach) adjusted before merge.
What actually fixes the crash
It's the try/catch, not the require → import switch. linux-distro@4 is CommonJS and internally does const execa = require('execa'), then calls execa('lsb_release', …) at line 11 (the crash site). How we load linux-distro from the outside doesn't change its own internal require('execa') — a dynamic import() of a CJS module still runs it through the CJS loader, and that internal require('execa') resolves identically. So if it was resolving the ESM execa@7 namespace (non-callable → execa is not a function) before, it still is after this change. The only reason the app stops crashing is that the synchronous throw is now caught.
Why this masks the bug rather than fixing it
With the current patch, linuxDistro() silently returns Unknown/Unknown on every affected build, and analytics.ts (data.os / data.release) then reports blank distro info. That's better than crashing, but it's graceful degradation, not restored detection.
Real root cause
An execa version mismatch: linux-distro@4 expects execa@^0.2.x (CJS, default-callable) but in the crashing environment resolves the hoisted execa@7 (ESM, named exports only). Neither import() nor the try/catch addresses that.
Suggested approach
- Keep the
try/catch(good defensive guard) — but rewrite the PR description to credit it, and drop or clearly mark therequire → importchange as incidental. - Actually fix detection with one of: ensure
linux-distroresolves a CJSexeca@0.2.x(nest/pin it), or — simplest and dependency-free — replacelinux-distrowith a direct read of/etc/os-release(ID/VERSION_ID), which removes the fragileexecachain entirely. - Make the fallback shape match what the consumer reads (
{ os, name, release, code }) so the degraded value is consistent instead ofundefined. - Smoke-test on a real
dist:linuxbuild to confirm esbuild preservesimport()and the main process executes it.
Happy to help with the /etc/os-release version if useful.
|
One more clarification on severity: this isn't actually an app crash — the IPC rejection is already caught in |
|
You’re right, I conflated preventing the crash with actually restoring distro detection. Dynamic import doesn’t change how |
…to#2236) Replace linux-distro dependency with direct /etc/os-release reading via fs.readFileSync. linux-distro@4 internally uses require('execa'), but the bundled execa is ESM-only (v7), causing TypeError at runtime. Switching from require() to dynamic import() doesn't fix this — the CJS module still resolves execa via CJS require internally. Removing linux-distro entirely eliminates the fragile execa dependency chain. Also fixes the return type to match the actual shape expected by analytics.ts consumer: { os, name, release, code } instead of the incorrect { name, version }.
252cef8 to
bc48a01
Compare
Description
Fixes #2236 — replaces the
linux-distronpm dependency with a direct read of/etc/os-release, eliminating the fragileexecaCJS/ESM interop chain that causedTypeError: execa is not a functionon Linux.What changed
Before:
require('linux-distro')which internally calledrequire('execa'). Since the bundledexecais ESM-only (v7), the CJSrequireresolved a module namespace object instead of a callable function, crashing at the call site on line 11 oflinux-distro/index.js.After: Read
/etc/os-release(or/usr/lib/os-release//etc/lsb-releaseas fallbacks) directly withfs.readFileSync. Parse theKEY=valueformat and return the{ os, name, release, code }shape thatanalytics.tsalready expects.Why not the previous approach
The earlier commit switched from
require()to dynamicimport(). This didn't fix the actual problem —linux-distro@4is still a CJS module, and its internalrequire('execa')resolves identically regardless of how we import it from the outside. Thetry/catchonly masked the crash while returning a wrong-shaped fallback. This version removes the dependency entirely.Verification
analytics.tsreadsdata.osanddata.release— both are now populated from/etc/os-release{ os: 'Linux', name: 'Unknown', release: '', code: '' }