Move export declarations to bottom of module files - #58
Conversation
- Relocate `export { ... }` blocks from the top to the bottom of files across `core`, `tasks`, and `task-explorer` packages for consistent module structure.
- Refactor `core/src/json/schema.ts` to use namespace imports (`decoder.X`, `encoder.X`) instead of `D`/`E` aliases and import `DecoderOptional`/`EncoderOptional` types directly.
- Simplify several `Schema` constructors in `schema.ts` by inlining intermediate `decoder`/`encoder` variables.
- Bump `@ambarltd/core` version to `0.1.15`.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
- Add `"packageManager": "pnpm@10.33.4"` to root, frontend, and backend `package.json` files. - Remove `corepack use pnpm@latest-10` invocations from all Dockerfile stages, letting corepack resolve the version from `packageManager`.
|
I suspect relocating all of the exports in all of the packages were unnecessary. Just making I had a similar problem when building task-explorer. Except the ESM build issue was caused by a third party dependency, and I didn't have the option to change the source code. Fixing duplicate imports & exports in My analysisThe PR correctly identifies the bug, but the fix is much broader than necessary. The crash has two specific causes, both isolated to schema.ts:
The export { ... } block placement (top vs bottom) is not the actual trigger. In spec-compliant ESM, export declarations are hoisted and linked before execution regardless of source position. The crash Alternative (minimal) fixes Option A — Just remove the duplicate aliases (smallest change, ~30 lines in 1 file) Option B — Use inline export on each declaration Option C — Publish ESM-only (or fix the build) Option D — Named imports instead of namespace imports Bottom line The real fix is Option A — it's exactly the schema.ts changes already in this PR. Moving exports to the bottom in the other 37 files is harmless but unnecessary; those files don't have duplicate namespace imports and wouldn't hit this Metro bug. It's a stylistic normalization, not a bug fix. |

Motivation
The mobile app in
HartAgencycrashes on sign-in withTypeError: Cannot read property 'object' of undefined, thrown from@ambarltd/core/dist/json/schema.jsat module init. The compiled file lists its aggregateexport { ... }clause ahead of the namespace imports it re-exports, and./decoder/./encoderare each imported twice (once asdecoder/encoder, once asD/E). Node's ESM loader links exports before any top-level code runs, so it survives this ordering. Metro lowers each module to a CJS-style factory and hoists the export object; under that transform the second namespace alias (D) resolves toundefinedwhenD.object(pdef)executes. Reordering the exports to the bottom and dropping the duplicate aliases removes the trigger so the same@ambarltd/corebuild runs cleanly under Metro, Node, esbuild, Rollup, and Webpack 5. Full write-up:docs/ambar-core-metro-bug.mdin the HartAgency repo.What's New
Export Ordering Refactor
export { ... }block from the top of every module file to the bottom incore,tasks,task-explorer/backend, andtask-explorer/frontendjson/schema.tsNamespace Cleanupimport * as D from "./decoder"andimport * as E from "./encoder"aliasesD.*/E.*reference to use the existingdecoder.*/encoder.*namespacesDecoderOptionalandEncoderOptionalas named types in place ofD.DecoderOptional/E.EncoderOptionalconst decoder = ...; const encoder = ...; return new Schema(decoder, encoder)inobject,pair, andtripleso the local bindings no longer shadow the module-scope namespace importsRelease
@ambarltd/coreto0.1.15so downstream consumers can drop the pnpm patch pinned against0.1.11ESM vs Metro Module Lowering
graph TD A[schema.ts source] --> B{Bundler} B -->|Node / esbuild / Rollup / Webpack 5| C[ESM link phase] B -->|Metro Babel transform| D[CJS-style factory] C --> E[Resolve all import / export bindings before any code runs] E --> F[Top-level statements execute in source order] F --> G[D.object pdef resolves correctly] D --> H[Hoist export object to top of factory] H --> I[Materialize first namespace import: decoder] I --> J[Second alias D for same module never assigned] J --> K[D undefined at call time] K --> L[TypeError: Cannot read property 'object' of undefined] style G fill:#163,color:#fff style L fill:#a22,color:#fffChanged Files
core/package.json0.1.14→0.1.15core/src/json/schema.tsD/Ealiases, inline shadowing bindings inobject,pair,triplecore/src/json/decoder.tscore/src/json/encoder.tscore/src/future.tscore/src/list.tscore/src/maybe.tscore/src/remote-data.tscore/src/result.tscore/src/router.tscore/src/time.tscore/src/tree-map.tscore/src/tree-set.tscore/tests/suites/json.tscore/tests/suites/list.tscore/tests/suites/time.tscore/tests/suites/tree-map.tscore/tests/suites/tree-set.tstasks/src/definition.tstasks/src/explorer.tstasks/src/postgres.tstasks/src/store.tstasks/src/worker.tstasks/tests/suites/postgres.tstasks/tests/suites/tasks/explorer.tstasks/tests/suites/tasks/helpers.tstasks/tests/suites/tasks/index.tstasks/tests/suites/tasks/store.tstasks/tests/suites/tasks/taskM.tstasks/tests/suites/tasks/worker.tstask-explorer/backend/src/lib/api-schemas.tstask-explorer/backend/src/lib/decoders.tstask-explorer/backend/tests/helpers.tstask-explorer/frontend/src/lib/api.tstask-explorer/frontend/src/lib/taskFormatUtils.tstask-explorer/frontend/src/lib/taskStatsUtils.tstask-explorer/frontend/src/lib/taskUtils.tstask-explorer/frontend/src/lib/useClickOutside.tstask-explorer/frontend/src/lib/useDebounce.tsTesting & Feedback
core,tasks, andtask-explorertest suites to confirm no regression from the namespace-alias rewrite incore/src/json/schema.ts@ambarltd/coreand install the0.1.15artifact into a Metro-bundled Expo / React Native app that imports@ambarltd/core/json/schemaat module top level; confirm noCannot read property 'object' of undefinedat module initdecode/encodeon at least oneobject,pair, andtripleschema to verify the inlined construction paths inschema.tsstill behave identicallyIf you find any bugs or have recommendations for improvements, please open an issue and assign it to me.