Skip to content

fix(build): let the published package use Deno's own Deno global - #3607

Merged
kojiwakayama merged 2 commits into
mainfrom
fix/dx-20260811-r2-19
Aug 12, 2026
Merged

fix(build): let the published package use Deno's own Deno global#3607
kojiwakayama merged 2 commits into
mainfrom
fix/dx-20260811-r2-19

Conversation

@kojiwakayama

@kojiwakayama kojiwakayama commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Symptom

The CLI was entirely non-functional under Deno, even though
https://veryfront.com/docs/code/getting-started/installation documents
deno add npm:veryfront as a supported install path.

$ deno run -A npm:veryfront@0.1.1229 dev --port 3795

✗ [unknown-error] Unknown/unclassified error
  Detail: Cannot read properties of null (reading 'fd')
  Suggestion: Check logs for more details

The same project ran fine under Node. Not a regression — 0.1.1228 was also
broken under Deno, with a different message.

Root cause

The boundary reported unknown-error and dropped the original stack. Logging
the raw throwable in the published package gives the real one:

TypeError: Cannot read properties of null (reading 'fd')
    at Object.listen (node_modules/@deno/shim-deno/dist/deno/stable/functions/listen.js:44:64)
    at isPortAvailable (node_modules/veryfront/esm/cli/commands/dev/port-fallback.js:38:26)
    at clearLocalCachesIfPortFree (node_modules/veryfront/esm/cli/commands/dev/command.js:95:16)

dnt emits npm/esm/_dnt.shims.js as an unconditional re-export:

import { Deno } from "@deno/shim-deno";
export { Deno } from "@deno/shim-deno";

So every framework module that touches Deno.* got the Node
reimplementation — including when Deno itself was executing the package. The
shim's Deno.listen builds a node:net server and immediately reads
server._handle.fd:

const server = createServer();
const waitFor = new Promise((resolve) => server.listen(port, hostname, resolve));
// server._handle.fd is assigned immediately on .listen()
const listener = new Listener(server._handle.fd, /* … */);

That assumption does not hold on Deno's node compatibility layer, where
_handle is null. isPortAvailable is the first thing dev does, so the
process died before the server bound. Any port-binding command (dev,
start, …) hit the same wall.

Fix

Patch the generated shim in postBuild, right beside the existing
process.argv[1] fix, so it defers to the host runtime's own Deno and keeps
@deno/shim-deno as the fallback for Node and Bun:

import { Deno as dntShimDeno } from "@deno/shim-deno";
const dntNativeDeno = globalThis.Deno;
export const Deno = typeof dntNativeDeno?.version?.deno === "string"
    ? dntNativeDeno
    : dntShimDeno;

The first-party extension packages build through dnt with the same
shims: { deno: true } config and carry an identical shim file, so they get
the same patch.

patchDntDenoShim fails closed: if dnt's output shape ever changes, the root
build throws rather than silently shipping the broken shim again.

Verification against the published repro

Negative control, published 0.1.1229 — deno run -A npm:veryfront@0.1.1229 dev --port 3795 in a veryfront init project outside the monorepo:

✗ [unknown-error] Unknown/unclassified error
  Detail: Cannot read properties of null (reading 'fd')

Same project, same command, with node_modules/veryfront replaced by this
branch's deno task build:npm output:

=== shim head of the package under test ===
import { Deno as dntShimDeno } from "@deno/shim-deno";
const dntNativeDeno = globalThis.Deno;

Veryfront (v0.1.1229)

  ✓ Ready in 1.0s
  http://veryfront.me:3795

--verbose confirms it now walks all the way through route discovery and
binds the port instead of aborting after "Using local filesystem (no proxy
mode)".

Tests

scripts/build/dnt-polyfill.test.ts (already in deno task test:scripts).
The tests evaluate the shim module for real, with @deno/shim-deno swapped
for a local stub, so they assert which Deno the module actually hands back
rather than matching on source text:

  • unpatched DNT shims shadow the real Deno global (the bug being fixed)
    pins the pre-fix behaviour; fails if the patch is ever dropped
  • patchDntDenoShim makes the published shims prefer the real Deno global
  • patchDntDenoShim keeps the shim fallback for runtimes without Deno
  • idempotency, fail-closed, and no-op-when-absent cases

Confirmed red before the fix (does not provide an export named 'patchDntDenoShim'), green after.

Scope

Build-script only; no src/ or docs changes, so nothing needs to be checked
on the live docs site for this PR.

Summary by CodeRabbit

  • Bug Fixes

    • Improved compatibility for generated packages running in environments with native Deno APIs.
    • Prevented generated compatibility shims from overriding available native functionality.
    • Added safer handling for missing, already-patched, or unexpected generated files.
  • Tests

    • Expanded coverage for compatibility behavior, fallback handling, idempotent patching, and missing-output scenarios.

`deno run -A npm:veryfront dev` aborted before binding a port with
"Cannot read properties of null (reading 'fd')", so the whole CLI was
unusable on the runtime our installation docs list as supported.

dnt emits `_dnt.shims.js` as an unconditional re-export of
`@deno/shim-deno`, so every framework module that touches `Deno.*` got
the Node reimplementation even when Deno itself was executing the
package. Those reimplementations are not runnable under Deno:
`Deno.listen` builds a node:net server and immediately reads
`server._handle.fd`, which is null on Deno's node compatibility layer.
`isPortAvailable` calls it first, so `dev`, `start`, and every other
port-binding command died there.

Patch the generated shim, next to the existing `process.argv[1]` fix, so
it prefers `globalThis.Deno` when the host runtime supplies one and falls
back to `@deno/shim-deno` for Node and Bun. The first-party extension
packages get the same treatment; they carry an identical shim file.
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds patchDntDenoShim, tests native-Deno preference and fallback behavior, and invokes the patcher in both npm build flows.

Changes

DNT shim patching

Layer / File(s) Summary
Native-Deno shim patcher
scripts/build/dnt-polyfill.ts
Adds patchDntDenoShim and its options type. The patcher replaces unconditional DNT shim exports, preserves native Deno, supports fallback behavior, and handles missing or unexpected output.
Shim patch validation
scripts/build/dnt-polyfill.test.ts
Adds BDD-style tests for native-Deno preference, fallback behavior, idempotence, required failures, and skipped files.
npm build integration
scripts/build/build-npm-dnt.ts, scripts/build/build-npm-extension-packages.ts
Invokes patchDntDenoShim while post-processing generated npm packages.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant NpmBuild as npm build flow
  participant ShimPatcher as patchDntDenoShim
  participant GeneratedShim as _dnt.shims.js
  participant Runtime as Native Deno global
  participant Fallback as `@deno/shim-deno`

  NpmBuild->>ShimPatcher: patch generated shim
  ShimPatcher->>GeneratedShim: replace unconditional export
  GeneratedShim->>Runtime: use native Deno when available
  GeneratedShim->>Fallback: use fallback otherwise
Loading

Possibly related PRs

Suggested reviewers: kwakayama

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: published packages now prefer Deno's native global.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/dx-20260811-r2-19

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfb24177e4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/build/dnt-polyfill.test.ts Outdated
@kwakayama kwakayama added needs-human-input Maintainer action required and removed needs-human-input Maintainer action required labels Aug 12, 2026
AGENTS.md requires describe()/it() from #veryfront/testing/bdd.ts and
assertions from #veryfront/testing/assert.ts. Converts the whole file,
not just the cases added by this PR, so it does not end up split across
two harnesses. Five sibling tests in scripts/build/ already import the
same specifiers under scripts/test.deno.json, which maps #veryfront/ to
../src/.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/build/build-npm-extension-packages.ts`:
- Line 93: Update the patchDntDenoShim call in the extension package build to
pass the required option, ensuring missing generated _dnt.shims.js output causes
the build to fail closed. Match the existing required-shim usage in
build-npm-dnt.ts.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 18f807c6-cefe-45b5-8c03-5bfb3b9e3be5

📥 Commits

Reviewing files that changed from the base of the PR and between 64d6850 and d715f8d.

📒 Files selected for processing (4)
  • scripts/build/build-npm-dnt.ts
  • scripts/build/build-npm-extension-packages.ts
  • scripts/build/dnt-polyfill.test.ts
  • scripts/build/dnt-polyfill.ts

Comment thread scripts/build/build-npm-extension-packages.ts

@kwakayama kwakayama left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No actionable findings found.

Evidence: scripts/build/dnt-polyfill.ts:11-19 preserves the Node/Bun shim fallback while selecting the native Deno namespace only for a real Deno runtime. The replacement is applied to the root artifact with a required shape guard at scripts/build/build-npm-dnt.ts:230-244, and to extension artifacts at scripts/build/build-npm-extension-packages.ts:91-93. Focused tests cover native selection, fallback, idempotence, and generated-shim drift at scripts/build/dnt-polyfill.test.ts:112-197.

Rubric Score
Correctness 40/40
Tests 19/20
Reliability/security 15/15
Maintainability 14/15
Scope/docs 10/10
Total 98/100

Review-Gate:
Reviewer: Codex
Reviewed-SHA: d715f8d
Score: 98/100
Actionable-Findings: 0
Verdict: APPROVE

@kojiwakayama
kojiwakayama added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit d27e8f6 Aug 12, 2026
33 checks passed
@kojiwakayama
kojiwakayama deleted the fix/dx-20260811-r2-19 branch August 12, 2026 05:05
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.

2 participants