Loading workspace insights... Statistics interval
7 days30 daysLatest CI Pipeline Executions
348220e7 feat(gonx): harden build-constraint context and tree-sitter WASM loading (#108, #113, #104) (#160)
## TL;DR
Three follow-ups from PR #106's review hardened the Go build-constraint
analyzer: policy (`cgo`, Go version) was hardcoded inside `matchTag` and
invisible from the type; the shared default `BuildContext` was mutable
and mapped cygwin to a non-existent GOOS; and the tree-sitter WASM
bytes-loading workaround had zero test coverage. This PR lifts that
policy onto `BuildContext`, freezes the default context (mapping cygwin
→ linux with a warning), and adds a contract test pinning
`Language.load(Uint8Array)` against an exact-pinned `web-tree-sitter`.
**Files to review (8, +266 / -32):**
| File | Why |
|---|---|
| `packages/gonx/src/graph/static-analysis/build-constraints.ts` *(start
here)* | `BuildContext` gains `cgoEnabled`/`goVersion`; `matchTag`
becomes pure; `getDefaultBuildContext` is frozen and cygwin-aware. |
| `packages/gonx/src/graph/static-analysis/build-constraints.spec.ts` |
New cgo/Go-version/freeze/cygwin tests; the old cygwin pass-through test
is rewritten to the new behavior. |
| `packages/gonx/src/graph/static-analysis/parser-init.spec.ts` *(new)*
| Contract test for the real WASM bytes path under Jest — the existing
suite mocks `web-tree-sitter`. |
| `packages/gonx/src/graph/static-analysis/parser-init.ts` | Documents
the `Language.load(bytes)` coupling and the pin/contract-test guarding
it. |
| `package.json`, `packages/gonx/package.json` | Pin `web-tree-sitter`
to exact `0.26.3` (repo resolution + published manifest). |
| `packages/gonx/docs/static-analysis.md` | Corrects a doc anchor and
documents the new context fields. |
## Why
These three issues were surfaced by PR #106's review and tracked as
follow-ups (#108, #113, #104). Five sibling issues from the same review
(#107, #109, #110, #111, #112) were already satisfied by code that
landed inside #106 and are now closed; this PR covers the remainder.
The concrete problems: a caller could not model "cgo is enabled" or
"this code compiles under Go 1.18" — `matchTag` answered `cgo → false`
and `go1.X → true` unconditionally. The default context was a plain
mutable object shared process-wide, and `cygwin` passed through as a
GOOS Go doesn't have, so `linux`/`unix`-gated files were wrongly
excluded on cygwin hosts. And the WASM workaround — reading bytes
ourselves to dodge Jest's dynamic-import failure — was exercised only
through mocks, so an upstream change to the bytes overload would have
broken graph construction silently.
## How
- **Policy onto the context (#108).** `BuildContext` gains optional
`cgoEnabled?: boolean` and a `goVersion?` field typed `1.${number}`.
`matchTag` now reads `cgo → ctx.cgoEnabled ?? false` and evaluates
`go1.M` against `ctx.goVersion` (satisfied when unset). Defaults
reproduce today's behavior exactly.
- **Harden the default (#113).** `getDefaultBuildContext` returns an
`Object.freeze`'d context with a frozen tags set; `nodePlatformToGoos`
special-cases `cygwin → linux` and warns once.
- **Contract-test the WASM path (#104).** A new `parser-init.spec.ts`
loads the real grammar from bytes and parses Go, with `web-tree-sitter`
pinned to exact `0.26.3` so the overload can't drift unseen.
## Reviewer notes
- **Default behavior is unchanged.** With `cgoEnabled`/`goVersion` unset
and no cygwin host, every existing result is identical — the 44 prior
`build-constraints` tests pass untouched. The new fields are purely
additive opt-ins.
- **Go versions compare numerically, not lexically.** `go1.10` under
`goVersion: '1.9'` must be *unsatisfied* (9 < 10); a string compare
would put `'1.9' > '1.10'` and wrongly include. A test pins this.
- **`Object.freeze` on a Set is partial by design.** It makes
`Object.isFrozen` true and blocks property addition, but the real
immutability guarantee for `tags` is the compile-time
`ReadonlySet<string>`; freeze is the runtime backstop the issue asked
for.
- **The cygwin warning fires once.** `extract-imports.ts` memoizes the
default context at module load, so the `logger.warn` is emitted a single
time per process, not per file.
- **The contract test deliberately un-mocks `web-tree-sitter`.** That is
the point — it runs the exact bytes path production uses under Jest, so
a breaking upgrade fails loudly here instead of silently dropping edges.
- **A malformed `goVersion` over-includes, never excludes.** The ``
`1.${number}` `` type can be subverted via `as`/external config;
`satisfiesGoVersion` validates `/^1\.\d+$/` and falls back to
satisfy-all rather than letting `NaN >= M` silently drop edges —
honoring the over-include policy. Validation of a user-facing
`goVersion` belongs at that future input boundary.
## Tests
`nx test gonx` → 278 passed (27 suites). New: cgo defaults (incl.
`!cgo`), Go-version policy (numeric boundary, legacy `// +build` path,
`&&` composition, unparseable over-include), cygwin/freeze, and the WASM
contract cases. `tsc --noEmit` clean; `eslint` 0 errors.
## Links
- [#108 — lift cgo/Go-version policy onto
BuildContext](https://github.com/naxodev/oss/issues/108)
- [#113 — freeze default context + cygwin
handling](https://github.com/naxodev/oss/issues/113)
- [#104 — harden WASM loading for tree-sitter under
Jest](https://github.com/naxodev/oss/issues/104)
- Parent review: [#106](https://github.com/naxodev/oss/pull/106) 2b61e80b refactor(nx-cloudflare): drop the createNodesV2 targets-cache (#164) (#166)
## TL;DR
The `createNodesV2` inference plugin memoized computed targets in a
`.nx/cache` file keyed by config path+content and plugin options — but
**not** the plugin's own code version. So upgrading
`@naxodev/nx-cloudflare` to a version that changes the inferred targets
served stale targets until `nx reset`. This drops the cache entirely.
## Why
The staleness is not hypothetical: it surfaced while implementing #137 —
editing `buildWorkerTargets` (adding `serve`'s `readyWhen`) returned a
prior run's cached targets until the cache file was manually cleared,
because the key can't capture "the plugin's logic changed."
Two fixes were possible: version the cache key, or drop the cache.
Dropping wins here because the cache we had was both **non-idiomatic**
and **cheap to do without**:
- **It was non-idiomatic.** Official Nx plugins that cache inference
(`@nx/vite`, `@nx/eslint`, `@nx/jest`) store under
`workspaceDataDirectory` and key via `calculateHashesForCreateNodes` — a
project-file hash **plus the lockfile** plus the options hash. Ours
stored under `cacheDir` and keyed only on `configFile + content` (no
lockfile). The lockfile component is what gives the official caches *de
facto* upgrade-invalidation (a plugin bump almost always moves the
lockfile hash); ours lacked it, so it was strictly more stale-prone than
the upstream pattern.
- **Inference here is trivial.** Per config: one `dirname`, one
directory read for the project-marker guard, one config read+parse to
validate, and building a small static targets object. The cache bought
marginal speed for real staleness risk plus
`readTargetsCache`/`writeTargetsCache` complexity.
This is a deliberate divergence from the official cache-the-targets
pattern, justified by the trivial cost — not a claim that caching is
wrong in general. `@naxodev/gonx`'s `createNodesV2` is likewise
uncached, so this also aligns the two plugins.
## How
`createNodesV2` now calls `createNodesFromFiles(createNodesInternal,
...)` directly — no cache file, no try/finally. `createNodesInternal`
builds targets inline. Removes the
`readTargetsCache`/`writeTargetsCache` helpers, the `TargetsCache` type,
and the
`cacheDir`/`node:crypto`/`readJsonFile`/`writeJsonFile`/`existsSync`
plumbing.
## Tests
12 unit tests pass; lint and build clean. The two cache-framed specs are
reworded to assert the underlying invariants (options flow through to
target names; multi-config isolation) without referencing the removed
cache. The old "repeated runs (cache safe)" test is dropped — with no
cache, `createNodesInternal` is a pure function of its inputs, so re-run
equality is tautological and can't fail on any plausible logic change
(the `typegen.cache: true` assertion it carried is already covered by
the richer five-target test). No behavior change to the inferred targets
themselves.
## Links
- Closes #164
- Cache originated in #136 (#158); staleness surfaced in #137 (#162) ·
Part of #115 c0df7b81 fix(nx-cloudflare): strip dangling @nx/node prune targets from generated worker projects (#163) (#165)
## TL;DR
The application generator already strips `@nx/node`'s `build`/`serve`
targets, but `@nx/node` *also* emits `prune`, `prune-lockfile`, and
`copy-workspace-modules` — all `dependsOn: ['build']`. With `build`
deleted these dangle (`nx prune <app>` can't resolve its dep) and are
meaningless for a Worker anyway, since Wrangler bundles on deploy. This
extends `removeNodeAppTargets` to delete them too.
## Why
Pre-existing leftover, not introduced here — the old `addTargets` also
deleted `build` while leaving these three. But #137's goal is a coherent
inferred-target Worker project, and these Node deployment helpers
undercut it: they reference a target that no longer exists.
Confirmed against `@nx/node@22.7.5` source: its application generator
unconditionally emits `build`, `serve`, `prune-lockfile`,
`copy-workspace-modules`, and `prune` (the prune chain all `dependsOn`
`build`), and exposes **no** option (`bundler`/`framework`) to suppress
them — so stripping by name after generation is the only available
approach.
## How
`removeNodeAppTargets` now deletes an explicit allowlist of the
`@nx/node` app targets that don't apply to a Worker — `build`, `serve`,
`prune`, `prune-lockfile`, `copy-workspace-modules` — and keeps
`lint`/`test`. The allowlist is a named constant so the intent (and
what's deliberately kept) is legible.
## Reviewer notes
- **The allowlist is coupled to `@nx/node`'s target set.** A future
`@nx/node` that adds another build-dependent target would silently
reintroduce this exact dangling-target bug. Guarded by a structural test
(below) that fails CI if any surviving target depends on a stripped one,
rather than relying on the list staying hand-synced.
## Tests
Three assertions, covering both failure directions of an allowlist:
1. The existing "without executor targets" test now also asserts
`prune`/`prune-lockfile`/`copy-workspace-modules` are absent. Verified
non-vacuous: with the three removed from the allowlist the test fails
(`@nx/node` does emit them).
2. A new test asserts `lint` is **kept** — guards against the allowlist
degrading into a denylist/over-broad loop that wipes targets the Worker
still needs.
3. A new structural test asserts no surviving target `dependsOn` a
stripped one — the future-`@nx/node` guard described above.
30 unit tests pass; lint and format clean.
## Links
- Closes #163
- Follow-up from #137 (#162) · Part of #115 070a79c2 feat(nx-cloudflare)!: replace serve/deploy executors with inferred targets (#137) (#162)
## TL;DR
The `serve`/`deploy` executors duplicated what the #136 inference plugin
now provides, and the generator still wrote explicit executor targets
into `project.json` that **shadowed** the inferred ones. This deletes
both executors and stops generating those targets, so
`serve`/`deploy`/`typegen`/`version-upload`/`tail` come solely from
`@naxodev/nx-cloudflare/plugin`.
**Files to review (31, +169 / −1049):**
| File | Why |
|---|---|
| `packages/nx-cloudflare/src/generators/init/generator.ts` *(start
here)* | Registers the inference plugin in `nx.json` so generated
workspaces get inferred targets. |
| `packages/nx-cloudflare/src/generators/application/generator.ts` |
`addTargets` → `removeNodeAppTargets` (strips `@nx/node`'s build+serve);
`port` now feeds the wrangler config. |
| `packages/nx-cloudflare/src/plugin.ts` | Inferred `serve` gains
`readyWhen: 'Ready on'` so dependents wait for the dev server to
actually listen. |
|
`packages/nx-cloudflare/src/generators/application/files/config/{jsonc,toml}/wrangler.*__tmpl__`
| Generated config gains `dev.port`. |
| `packages/nx-cloudflare/package.json`, `project.json`,
`.eslintrc.json` | Drop the executors manifest entirely (no executors
ship); keep `wrangler` as a CLI runtime dep in dependency-checks. |
| `packages/nx-cloudflare-e2e/src/application.spec.ts` | Asserts
inferred targets instead of executors — the first true end-to-end
exercise of #136. |
| `packages/nx-cloudflare/README.md` | Documents inferred targets; drops
the executor option tables. |
## Why
Nx 22 Crystal favors inferring CLI-wrapping targets over bespoke
executors. The `serve` executor hand-rolled continuous-server plumbing
(`createAsyncIterable`, `waitForPortOpen`, signal handling) that
`continuous: true` now provides for free; `deploy` was a thin `wrangler
deploy` wrapper. #136 already infers all five targets — but **explicit
`project.json` targets win over inferred ones in Nx**, so for generated
apps the inference was inert until those explicit targets were removed.
This completes that migration.
## How
The generator's `init` step registers `@naxodev/nx-cloudflare/plugin` in
`nx.json` (idempotent), so a freshly generated app immediately gets
inferred targets. The application generator no longer writes
`serve`/`deploy`; it deletes the `build` and `serve` targets
`@nx/node`'s generator adds (neither applies to a Worker). The `port`
option moves into the generated wrangler config's `dev.port`, which
`wrangler dev` reads. With nothing left referencing them, both executors
and their four executor-only utilities are deleted.
## Reviewer notes
- **Breaking — pairs with #138.** Existing workspaces' `project.json`
files reference the deleted executors and break until #138's migration
strips those targets and registers the plugin. **#137 must ship in the
same release as #138.**
- **`removeNodeAppTargets` deletes `@nx/node`'s `build` *and* `serve`.**
The node generator emits an `@nx/js:node` `serve` (`dependsOn:
['build']`); with `build` gone it would both break and shadow the
inferred `serve`. The old `addTargets` masked this by overwriting
`serve`.
- **`serve` signals readiness via `readyWhen: 'Ready on'`.** The deleted
executor used `waitForPortOpen` to report ready only once the port was
listening; bare `continuous: true` doesn't gate on that. `wrangler dev`
prints `[wrangler:info] Ready on http://...` when listening (verified
against wrangler 4.26), so dependents wait correctly.
- **No executors ship, so no executors manifest.** Rather than an empty
`{ "executors": {} }`, the `executors` field, `executors.json`, and the
build/exports/lint references are removed — the Nx-recommended shape for
a generators+inference plugin (an empty manifest is vestigial).
`@naxodev/gonx` keeps its `executors.json` because its inference points
at real executors; nx-cloudflare deliberately infers `nx:run-commands`
instead.
- **`port` default is now `8787`, not `3000`.** This aligns
`normalizeOptions` with the schema's declared default (and Wrangler's),
fixing a pre-existing code/schema mismatch.
- **`wrangler` stays a `peerDependency` but is ignored by
`@nx/dependency-checks`.** The inferred targets invoke the `wrangler`
CLI (`externalDependencies: ['wrangler']`), but nothing imports it
statically anymore, so the dependency check needs the explicit ignore.
## Tests
82 unit tests pass. New/updated coverage: init registers the plugin
(idempotent); the application generator writes no
`serve`/`deploy`/`build` targets and emits `dev.port` (default 8787);
the e2e generates an app and asserts
`serve`/`deploy`/`typegen`/`version-upload`/`tail` are inferred (with
`serve` continuous) and not the old executor. The three e2e serve-runner
tests are unchanged — `nx serve` now runs `wrangler dev`, which still
prints `Starting local server`.
## Follow-up
- **#138** — migration to strip the old executor targets from existing
`project.json` files and register the plugin in existing workspaces.
- **#163** — strip `@nx/node`'s dangling
`prune`/`prune-lockfile`/`copy-workspace-modules` targets (pre-existing;
`dependsOn` the now-deleted `build`).
- **#164** — invalidate the createNodesV2 targets-cache on plugin
version change (cache key omits the plugin code version, so upgrades
need `nx reset`; pre-existing from #136).
## Links
- Closes #137
- Builds on #136 (#158)
- Epic #115