Full migratione from cjs to es#1573
Conversation
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive migration of the codebase to ECMAScript Modules (ESM). Key changes include updating package.json files to specify module types, refactoring source files from CommonJS to ESM syntax, and introducing a new resolve-helper utility for cross-environment package resolution. The review feedback focuses on the resolve-helper implementation, specifically addressing a bug in subpath wildcard replacement, improving error code accuracy for unexported paths, and suggesting a naming convention update for constants.
| const middle = subpath.slice(prefix.length, subpath.length - (suffix?.length ?? 0)); | ||
| const target = resolveConditions(obj[key], subpath); | ||
| if (typeof target !== "string") return target; | ||
| return target.replace("*", middle); |
There was a problem hiding this comment.
Node.js subpath patterns allow the * wildcard to be used multiple times in the exports target string. Using replace only substitutes the first occurrence. Use replaceAll to ensure all instances are correctly replaced, which is the standard behavior in Node.js resolution.
| return target.replace("*", middle); | |
| return target.replaceAll("*", middle); |
| } | ||
| } | ||
| } | ||
| return subpath === "." ? undefined : null; |
There was a problem hiding this comment.
When the exports field is present in package.json, any subpath that is not explicitly matched should result in an ERR_PACKAGE_PATH_NOT_EXPORTED error rather than MODULE_NOT_FOUND. In this implementation, returning undefined from matchExports leads to MODULE_NOT_FOUND in esmResolve, while null correctly signals the export error. This fallback (and others at lines 115, 123, and 159) should return null to better align with Node's resolution algorithm.
| return subpath === "." ? undefined : null; | |
| return null; |
| return undefined; | ||
| } | ||
|
|
||
| const selfRequire = createRequire(import.meta.url); |
There was a problem hiding this comment.
According to the general rules, constants should use SCREAM_CASE or UpperCamelCase. Please rename selfRequire to follow this convention (e.g., SELF_REQUIRE). Note that you will also need to update its usage on line 189.
| const selfRequire = createRequire(import.meta.url); | |
| const SELF_REQUIRE = createRequire(import.meta.url); |
References
- Use SCREAM_CASE or UpperCamelCase for constants.
| import * as blockTools from '@platforma-sdk/block-tools'; | ||
|
|
||
| async function loadBlockDescription() { | ||
| const __dirname = dirname(fileURLToPath(import.meta.url)); |
There was a problem hiding this comment.
import.meta.dirname couldn't be used here?
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (res !== undefined) return res; | ||
| } | ||
| } | ||
| if ("default" in obj) return resolveConditions(obj.default, subpath); |
There was a problem hiding this comment.
"default" is already handled by the CONDITIONS loop
CONDITIONS is ["node", "import", "default"], so when the loop at lines 175–180 runs, it already tries "default" and — if found — calls resolveConditions(obj.default, subpath). If that call returns undefined, the loop finishes and falls through; the if ("default" in obj) guard on line 181 is then entered, but the call returns exactly the same undefined value. This line can never alter the final result and is unreachable in any meaningful way.
| if ("default" in obj) return resolveConditions(obj.default, subpath); | |
| return undefined; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/node/resolve-helper/src/index.ts
Line: 181
Comment:
**Dead code — `"default"` is already handled by the `CONDITIONS` loop**
`CONDITIONS` is `["node", "import", "default"]`, so when the loop at lines 175–180 runs, it already tries `"default"` and — if found — calls `resolveConditions(obj.default, subpath)`. If that call returns `undefined`, the loop finishes and falls through; the `if ("default" in obj)` guard on line 181 is then entered, but the call returns exactly the same `undefined` value. This line can never alter the final result and is unreachable in any meaningful way.
```suggestion
return undefined;
```
How can I resolve this? If you propose a fix, please make it concise.| function migrateGlobalOverviewEntry<D>(raw: unknown): GlobalOverviewEntry<D> { | ||
| if (!raw || typeof raw !== "object") | ||
| throw new Error(`Invalid global overview entry: ${JSON.stringify(raw)}`); | ||
| const o = raw as Record<string, unknown> & Partial<GlobalOverviewEntry<D>>; | ||
| const allVersionsWithChannels = | ||
| o.allVersionsWithChannels ?? (o.allVersions ?? []).map((v) => ({ version: v, channels: [] })); | ||
| const latestByChannel = { ...(o.latestByChannel ?? {}) }; | ||
| if (!latestByChannel[AnyChannel] && o.latest !== undefined && o.latestManifestSha256) { | ||
| latestByChannel[AnyChannel] = { | ||
| description: o.latest as D, | ||
| manifestSha256: o.latestManifestSha256, | ||
| }; | ||
| } | ||
| return { | ||
| ...o, | ||
| id: o.id as BlockPackIdNoVersion, | ||
| allVersionsWithChannels, | ||
| latest: o.latest as D, | ||
| latestManifestSha256: (o.latestManifestSha256 ?? "") as string, | ||
| latestByChannel, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The previous Zod-based GlobalOverviewReg.parse() validated every field including id, description objects (BlockPackDescriptionManifest), and SHA256 hashes, throwing a structured error on any mismatch. The new migrateGlobalOverviewEntry just casts these with as:
id: o.id as BlockPackIdNoVersion— ifo.idisundefinedor malformed, the cast succeeds but downstream accesses onid.organization/id.namewill throw crypticTypeErrors rather than a clear parse error.descriptionvalues insidelatestByChannelentries are not checked againstBlockPackDescriptionManifestat all.
Consider adding at least a guard for the id field, e.g.:
if (!o.id || typeof o.id !== "object")
throw new Error(`Invalid global overview entry: missing or invalid id`);Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/block-tools/src/v2/registry/schema_public.ts
Line: 98-119
Comment:
**Loss of runtime validation for required fields**
The previous Zod-based `GlobalOverviewReg.parse()` validated every field including `id`, description objects (`BlockPackDescriptionManifest`), and SHA256 hashes, throwing a structured error on any mismatch. The new `migrateGlobalOverviewEntry` just casts these with `as`:
- `id: o.id as BlockPackIdNoVersion` — if `o.id` is `undefined` or malformed, the cast succeeds but downstream accesses on `id.organization` / `id.name` will throw cryptic `TypeError`s rather than a clear parse error.
- `description` values inside `latestByChannel` entries are not checked against `BlockPackDescriptionManifest` at all.
Consider adding at least a guard for the `id` field, e.g.:
```typescript
if (!o.id || typeof o.id !== "object")
throw new Error(`Invalid global overview entry: missing or invalid id`);
```
How can I resolve this? If you propose a fix, please make it concise.| const require = createRequire(import.meta.url); | ||
| const resolved = require.resolve(modulePath); | ||
| // eslint-disable-next-line prefer-const, @typescript-eslint/no-unsafe-assignment | ||
| let { model, platforma } = require(modulePath); | ||
| let { model, platforma } = await import(pathToFileURL(resolved).href); |
There was a problem hiding this comment.
require.resolve may fail for pure-ESM model packages with an exports field
createRequire(import.meta.url).resolve(modulePath) uses the CJS resolution algorithm, which only honours require / node export conditions. If a model package under flags.modulePath exposes only an import condition (e.g. "exports": { ".": { "import": "./dist/index.js" } }), require.resolve will throw ERR_PACKAGE_PATH_NOT_EXPORTED and the import() call is never reached.
The current model packages (e.g. enter-numbers/model) rely solely on "main" (no exports field), so this works today. But as packages migrate further, this becomes fragile. Using import.meta.resolve (Node ≥ 20.6) or the workspace's own tryResolve helper from @milaboratories/resolve-helper would handle both CJS and ESM exports correctly.
// Safer alternative (Node ≥ 20.6 or polyfilled via resolve-helper):
const resolved = await import.meta.resolve(pathToFileURL(modulePath).href);
let { model, platforma } = await import(resolved);Prompt To Fix With AI
This is a comment left during a code review.
Path: tools/block-tools/src/cmd/build-model.ts
Line: 48-51
Comment:
**`require.resolve` may fail for pure-ESM model packages with an `exports` field**
`createRequire(import.meta.url).resolve(modulePath)` uses the CJS resolution algorithm, which only honours `require` / `node` export conditions. If a model package under `flags.modulePath` exposes only an `import` condition (e.g. `"exports": { ".": { "import": "./dist/index.js" } }`), `require.resolve` will throw `ERR_PACKAGE_PATH_NOT_EXPORTED` and the `import()` call is never reached.
The current model packages (e.g. `enter-numbers/model`) rely solely on `"main"` (no `exports` field), so this works today. But as packages migrate further, this becomes fragile. Using `import.meta.resolve` (Node ≥ 20.6) or the workspace's own `tryResolve` helper from `@milaboratories/resolve-helper` would handle both CJS and ESM exports correctly.
```typescript
// Safer alternative (Node ≥ 20.6 or polyfilled via resolve-helper):
const resolved = await import.meta.resolve(pathToFileURL(modulePath).href);
let { model, platforma } = await import(resolved);
```
How can I resolve this? If you propose a fix, please make it concise.
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
2 participants
Greptile Summary
This PR completes the migration of 97 files across the monorepo from CommonJS to ESM, changing the default
createRolldownNodeConfigoutput from["es", "cjs"]to["es"], adding"type": "module"to package manifests, polyfilling__dirnamein CLI entry points, and replacing the hand-written dual-formatresolve-helperfiles with a single TypeScript source that implements a two-stage CJS→ESM resolver. The most notable functional change is the replacement of Zod-basedGlobalOverviewRegschema validation with a manual migration function that skips inner-field validation.Confidence Score: 5/5
Safe to merge; all findings are P2 style/robustness concerns that do not affect current functionality.
All three inline comments are P2: dead code in the ESM resolver, a loss of deep runtime validation (intentional trade-off for removing Zod from the parse path), and a future fragility in
require.resolvefor model packages that don't yet have exports fields. No regressions in the primary migration path are evident.lib/node/resolve-helper/src/index.ts (dead code), tools/block-tools/src/v2/registry/schema_public.ts (validation regression)
Important Files Changed
"default"condition checked twice).idand description fields.require()tocreateRequire().resolve()+ dynamicimport()for ESM support; fragile for future pure-ESM model packages with exports field.["es", "cjs"]to["es"]— the central ESM-only change for the build infrastructure.__filename/__dirnameESM polyfill needed byoclif.execute({ dir: __dirname }).src/index.js/.mjsdual output to a properts-builder-built ESM package; added standard build scripts and engine requirementnode >= 22.ts-builderESM-only build; updated oclif target path from./dist/cli.jsto./dist/cmd/index.js.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "full migration from cjs to es" | Re-trigger Greptile