ReviewOS

stacks/stx

convertToCommonJS rewrites a default import to the module namespace, so every property read is undefined — silently disables config-gated features

#1910
Closed glennmichael123 opened this 14 days ago · 1 comment
14 days ago

Summary

convertToCommonJS rewrites a default import to the module namespace, dropping .default:

// variable-extractor.ts:1248-1251
if (defaultImportMatch) {
  const [, name, source] = defaultImportMatch
  const resolved = resolveSource(source)
  convertedLines.push(`const ${name} = await import('${resolved}')`)
  convertedLines.push(`module.exports.${name} = ${name};`)
}

import x from 'y' must become (await import('y')).default. As written, x is the namespace object, so every property read off it is undefined — no throw, no warning. The named-import branch below it is correct; only the default branch is affected.

Reproduction

mod.ts:

export default { github: { clientId: 'GH_ID' } }

a.stx — the documented, correct spelling:

<script server>
import services from './mod'
const gh = services?.github?.clientId ?? 'UNDEFINED'
</script>
<p>{{ gh }}</p>

b.stx — the hand-written form:

<script server>
const services = (await import('./mod')).default
const gh = services?.github?.clientId ?? 'UNDEFINED'
</script>
<p>{{ gh }}</p>
static-default:    UNDEFINED
explicit-.default: GH_ID

Why this is worth fixing rather than documenting

The failure is silent and the guidance actively points at it. Our standards doc has a MUST rule — use import, never a hand-written await import() — with a real rationale: the rewrite is line-based, so a multi-line import kills the whole block and degrades to regex scraping. So an app is told to convert, converts, and gets undefined.

In our case both auth pages do:

const services = (await import('../../config/services')).default
const githubEnabled = !!services.github?.clientId

githubEnabled gates whether the OAuth button renders. Following the rule removes sign-in-with-GitHub and sign-in-with-Google from the login and register pages, on a 200, with nothing logged. We caught it only because we probed the rewrite before converting.

@config-style default-exporting modules are the normal shape for framework config, so this is likely to hit any app that adopts the rule.

Suggested fix

convertedLines.push(`const ${name} = (await import('${resolved}')).default`)

Worth deciding whether to preserve interop for a module with no default export (CommonJS interop usually falls back to the namespace). A test asserting import x from and (await import()).default produce the same binding for a default-exporting module would pin it either way.

Related: the same file's named-import branch carries a comment about a renaming bug that produced a SyntaxError and made every binding come back undefined with nothing logged — same failure signature, one branch over.

14 days ago

Fixed in 2e56a20964.

import x from 'y' now binds (await import('y')).default. Your repro returns GH_ID, and the documented spelling agrees with the hand-written (await import('./mod')).default — which is the part that made this nasty: the form the docs recommend was the broken one, and the workaround worked, so the two disagreed silently.

One correction to my own first fix, since it is the kind of thing worth writing down. I wrote ?.default ?? namespace, reasoning that a CommonJS module has its exports on the namespace with no .default. Measured, Bun does not behave that way:

./cjs.cjs   keys=["default","plain"]  hasDefault=true
./cjs.ts    keys=["default","plain"]  hasDefault=true
./esm.ts    keys=["default"]          hasDefault=true

It synthesises .default for CJS too, set to module.exports. So the fallback was unreachable — and sabotaging it changed nothing, because the test I wrote for it passed either way. A branch that cannot run is a claim about the runtime that nothing checks, so it is gone.

4 tests: the documented and hand-written spellings agree, a CJS module still reads, named imports untouched.

Sign in to comment on this issue.