Skip to content

spike: slim facade — bundler comparison (sum/enter-numbers-v3)#1644

Draft
dbolotin wants to merge 2 commits into
mainfrom
spike/slim-facade
Draft

spike: slim facade — bundler comparison (sum/enter-numbers-v3)#1644
dbolotin wants to merge 2 commits into
mainfrom
spike/slim-facade

Conversation

@dbolotin

Copy link
Copy Markdown
Member

Summary

Branch-only scratch from the slim-facade spike (Phases A+B). Bundles each test block's facade with both dts-bundle-generator and rolldown-plugin-dts for side-by-side inspection.

Not for merge — productionization plan lives in docs/mitxt/work/2026-05-13-test-framework/facade-build-plan.md (separate repo). This PR is the empirical artifact backing that plan.

What's in

Per-block (sum-numbers-v3, enter-numbers-v3):

  • block/src/index.ts — facade source. Exports BlockSpec (block-named twin + universal alias), OutputsOf / DataOf / HrefOf generators, BlockData, hand-written model helpers, placeholder blockSpec value.
  • block/tsconfig.json — declaration-emit tsconfig extending @milaboratories/ts-configs/node.
  • block/build-facade.mjs — scratch dual-bundler driver invoked by pnpm build:facade. Outputs land in dist-dbg/ (dts-bundle-generator) and dist-rdp/ (rolldown-plugin-dts); both gitignored.
  • block/package.json — local devDeps for the two bundlers and TS.
  • model/src/index.ts — Zod removed (not in use). BlockData hand-authored with per-field JSDoc. SumOutcome (sum-numbers-v3 only) added as a JSDoc-propagation probe through InferRenderFunctionReturn.

Findings (short)

  • Bundler choice: rolldown-plugin-dts. More aggressive default inlining (external: () => false → zero external imports). Same toolchain we already use in ts-builder.
  • JSDoc preservation: parity between the two bundlers on every probe.
  • DataOf<BlockSpec> propagation works end-to-end — BlockData alias survives through PlatformaV3<BlockData, …> slot, JSDoc reaches consumer.
  • OutputsOf<BlockSpec>[K] loses alias regardless of bundler — root cause is SDK-level expansion in InferRenderFunctionReturn<RF> during model .d.ts emit. SDK reshape needed; tracked separately.

Full Phase B results table and findings in spike-slim-facade-plan.md (docs/mitxt/).

What's not in

  • No ts-builder target — productionization is the next step, not this PR.
  • No JS-side facade rewrite — block/index.js untouched.
  • No cross-block test — folded into the productionization plan's test list.

dbolotin added 2 commits May 18, 2026 11:20
Branch-only scratch driver building each block's facade with both
`dts-bundle-generator` and `rolldown-plugin-dts`. Outputs land in
`dist-dbg/` and `dist-rdp/` (gitignored) for side-by-side inspection.

Per-block changes:

- `block/src/index.ts` — new facade source. Re-exports `BlockSpec`
  (block-named twin + universal alias), `OutputsOf` / `DataOf` /
  `HrefOf` type generators, `BlockData`, model helpers
  (`SumNumbersRef` / `EnterNumbersRef`, `SumOutcome` /
  `SortedNumbers`), and a placeholder `blockSpec` value.
- `block/tsconfig.json` — facade tsconfig extending
  `@milaboratories/ts-configs/node`, declaration emit only.
- `block/build-facade.mjs` — scratch dual-bundler driver invoked by
  `pnpm build:facade`.
- `block/package.json` — local devDeps for the two bundlers and TS.
- `model/src/index.ts` — Zod removed (operator: not in use). `BlockData`
  hand-authored with per-field JSDoc. One named output return type
  (`SumOutcome` / `SortedNumbers`) added as a JSDoc-propagation probe.

Findings summary lives in
`docs/mitxt/work/2026-05-13-test-framework/spike-slim-facade-plan.md`
(Phase B Results) and the productionization plan in `facade-build-plan.md`
in the same folder. Decision: `rolldown-plugin-dts` wins.
@changeset-bot

changeset-bot Bot commented May 18, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 9f69e48

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request implements a slim-facade spike for the enter-numbers-v3 and sum-numbers-v3 blocks, introducing build scripts that use rolldown and dts-bundle-generator to produce self-contained bundled facades. It replaces zod-based data models with native TypeScript types and adds reference shapes for block identity. Review feedback focuses on improving cross-platform compatibility by using fileURLToPath from node:url for directory resolution and ensuring constants follow standard naming conventions.

@@ -0,0 +1,59 @@
import type { InferDataType, InferHrefType, InferOutputsType } from "@platforma-sdk/model";

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.

medium

To ensure cross-platform compatibility, especially on Windows, it is recommended to use fileURLToPath from the node:url module when deriving file system paths from import.meta.url. This avoids issues with URI-encoded characters and the leading slash present in .pathname on Windows.

import { fileURLToPath } from "node:url";
import type { InferDataType, InferHrefType, InferOutputsType } from "@platforma-sdk/model";


export type { BlockData, EnterNumbersRef };

const __facadeDir__ = new URL(".", import.meta.url).pathname;

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.

medium

Using .pathname on a URL object returns a URI-style path (e.g., /C:/path on Windows), which is not a valid native path for Node.js file system APIs. Use fileURLToPath to obtain a platform-native path. Additionally, rename the constant to FACADE_DIR to adhere to the repository's naming convention (SCREAM_CASE or UpperCamelCase).

Suggested change
const __facadeDir__ = new URL(".", import.meta.url).pathname;
const FACADE_DIR = fileURLToPath(new URL(".", import.meta.url));
References
  1. Use SCREAM_CASE or UpperCamelCase for constants.

@@ -0,0 +1,60 @@
import type { InferDataType, InferHrefType, InferOutputsType } from "@platforma-sdk/model";

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.

medium

To ensure cross-platform compatibility, especially on Windows, it is recommended to use fileURLToPath from the node:url module when deriving file system paths from import.meta.url.

import { fileURLToPath } from "node:url";
import type { InferDataType, InferHrefType, InferOutputsType } from "@platforma-sdk/model";


export type { BlockData, SumNumbersRef, SumOutcome };

const __facadeDir__ = new URL(".", import.meta.url).pathname;

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.

medium

Using .pathname on a URL object returns a URI-style path which can cause issues on Windows. Use fileURLToPath for a platform-native path. Additionally, rename the constant to FACADE_DIR to adhere to the repository's naming convention (SCREAM_CASE or UpperCamelCase).

Suggested change
const __facadeDir__ = new URL(".", import.meta.url).pathname;
const FACADE_DIR = fileURLToPath(new URL(".", import.meta.url));
References
  1. Use SCREAM_CASE or UpperCamelCase for constants.

@codecov

codecov Bot commented May 18, 2026

Copy link
Copy Markdown

❌ 3 Tests Failed:

Tests completed Failed Passed Skipped
1755 3 1752 20
View the full list of 3 ❄️ flaky test(s)
src/ml-v3.test.ts > v3: limbo test

Flake rate in main: 50.00% (Passed 1 times, Failed 1 times)

Stack Traces | 7.27s run time
AssertionError: expected { ok: true, …(2) } to strictly equal { ok: true, value: 6, stable: true }

- Expected
+ Received

  {
    "ok": true,
    "stable": true,
-   "value": 6,
+   "value": {
+     "count": 1,
+     "sum": 6,
+   },
  }

 ❯ src/ml-v3.test.ts:480:48
 ❯ src/with-ml.ts:28:7
 ❯ Module.withTempRoot ../...../src/test/test_config.ts:231:21
 ❯ withMl src/with-ml.ts:13:3
 ❯ src/ml-v3.test.ts:443:3
src/ml-v3.test.ts > v3: simple project manipulations test

Flake rate in main: 50.00% (Passed 1 times, Failed 1 times)

Stack Traces | 12.8s run time
AssertionError: expected { ok: true, …(2) } to strictly equal { ok: true, value: 18, stable: true }

- Expected
+ Received

  {
    "ok": true,
    "stable": true,
-   "value": 18,
+   "value": {
+     "count": 2,
+     "sum": 18,
+   },
  }

 ❯ src/ml-v3.test.ts:253:48
 ❯ src/with-ml.ts:28:7
 ❯ Module.withTempRoot ../...../src/test/test_config.ts:231:21
 ❯ withMl src/with-ml.ts:13:3
 ❯ src/ml-v3.test.ts:133:3
src/v3.test.ts > v3: project watcher test

Flake rate in main: 50.00% (Passed 1 times, Failed 1 times)

Stack Traces | 6.51s run time
AssertionError: expected { ok: true, …(2) } to strictly equal { ok: true, value: 6, stable: true }

- Expected
+ Received

  {
    "ok": true,
    "stable": true,
-   "value": 6,
+   "value": {
+     "count": 1,
+     "sum": 6,
+   },
  }

 ❯ src/v3.test.ts:218:48
 ❯ src/with-ml.ts:28:7
 ❯ Module.withTempRoot ../...../src/test/test_config.ts:231:21
 ❯ withMl src/with-ml.ts:13:3
 ❯ src/v3.test.ts:155:3

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

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.

1 participant