fix(core): add byline to generated collection interfaces (Fixes #2888) - #2889
Conversation
🦋 Changeset detectedLatest commit: b56f8ee The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach is correct: getEmDashCollection / getEmDashEntry already attach entry.data.byline at runtime, so the generated collection interfaces should expose it. The fix is minimal and consistent with the existing bylines? and terms? hydrated fields.
I checked:
packages/core/src/query.ts—hydrateEntryBylinessetsdata.byline = credits[0]?.byline ?? nullon both the folded fast-path and the query path.packages/core/src/database/repositories/types.tsandpackages/core/src/index.ts—BylineSummaryis a real exported type from theemdashpackage.packages/core/src/schema/zod-generator.ts— the newbyline?: BylineSummary | null;line and the added import are correctly emitted for every generated interface.- Tests cover the new property and import.
- The slug
bylineis already inRESERVED_FIELD_SLUGS, so it cannot collide with user fields.
Headline: code is fine, but the changeset could be more reader-facing, and the committed emdash-env.d.ts files in templates/demos/fixtures are now stale and won't expose the new type until the dev server regenerates them.
Findings
-
[suggestion]
.changeset/type-generator-byline-2888.md:5The changeset is technically accurate, but it leads with an internal implementation detail ("the type generator") and explains the fix in terms of matching runtime functions. A CHANGELOG entry should lead with the observable surface so readers can tell whether the release affects them.
Generated `emdash-env.d.ts` files now include `entry.data.byline?: BylineSummary | null` alongside `bylines`, matching the byline data hydrated by `getEmDashEntry` and `getEmDashCollection` at runtime. -
[suggestion]
templates/blog/emdash-env.d.ts:6-17This committed, generated file (and the matching files in
templates/*,demos/*,fixtures/perf-site,e2e/fixture*, etc.) is now stale: it importsContentBylineCreditbut notBylineSummary, and the generated interfaces omitbyline?: BylineSummary | null;. Until the dev server regenerates them, users starting from these templates won't see the type-safebylinefield that this PR adds. Consider runningpnpm dev(or the typegen flow) in each tracked project and committing the regeneratedemdash-env.d.tsfiles.Example expected diff for this file:
import type { BylineSummary, ContentBylineCredit, TaxonomyTerm, PortableTextBlock } from "emdash";and add
byline?: BylineSummary | null;next to each existingbylines?: ContentBylineCredit[];line.
github-actions
Bot
added
review/awaiting-author
github-actions
Bot
added
review/needs-rereview
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
|
@amasen02 Thanks for picking up #2888 so quickly after I opened it — fix looks solid, and the regenerated template types address exactly what That said, I think the Overlapping PRs bot flag here is a false positive and shouldn't have been a reason to close this PR. #1099 and #2889 solve completely different problems:
The only overlap is 4 shared
Both PRs touch these because they're auto-generated committed type files — not because the changes are related. #1099 updates them as a side effect of template work; #2889 regenerates them to include the new A merge conflict in generated files is trivial to resolve (regenerate after merge). There's no duplicated work or conflicting intent.
|
|
Reopened! Thanks for the context on #1099 and clarifying that the overlap warning was just on the generated emdash-env.d.ts files. All checks are green and ready whenever you're set to merge. |
There was a problem hiding this comment.
The approach remains correct: the generated interfaces should expose the byline field because getEmDashCollection/getEmDashEntry already attach it at runtime, and BylineSummary is a real export from the emdash package. The core change in zod-generator.ts is minimal and consistent with the existing bylines?/terms? hydrated fields, and the new test covers the import and property.
The prior review asked for a reader-facing changeset and regenerated emdash-env.d.ts files. The author has addressed both: the changeset now leads with the generated-file surface, and all 23 tracked emdash-env.d.ts files were updated with the BylineSummary import and byline property. Core code and tests look fine.
One regenerated file is malformed, though: templates/blog/emdash-env.d.ts has duplicated byline?: BylineSummary | null; declarations in both the Page and Post interfaces. It needs to be regenerated cleanly or the duplicates removed so the committed file matches the generator output.
| byline?: BylineSummary | null; | ||
| byline?: BylineSummary | null; |
There was a problem hiding this comment.
[needs fixing] The Page interface here has two consecutive byline?: BylineSummary | null; declarations, and the Post interface has the same duplicate at lines 33–34. TypeScript will merge identical interface members, but the file header says it is generated by EmDash and should not be edited manually. The zod-generator.ts change emits only one byline line per interface, so this file no longer matches generator output and was clearly patched by hand.
Regenerate the file (e.g. pnpm dev in templates/blog) or remove the duplicate lines so it matches the output of generateTypesFile.
| byline?: BylineSummary | null; | |
| byline?: BylineSummary | null; | |
| publishedAt: Date | null; | |
| byline?: BylineSummary | null; | |
| bylines?: ContentBylineCredit[]; | |
| terms?: Record<string, TaxonomyTerm[]>; | |
| } |
(Apply the same deletion in the Post interface at lines 33–34.)
|
Pushed commit \97966fd5\ removing the duplicate \�yline\ property declarations in \ emplates/blog/emdash-env.d.ts\ for both \Page\ and \Post\ interfaces. Verified unit test suite (\packages/core/tests/unit/schema/zod-generator.test.ts) passes 58/58 cleanly. |
|
@gornostay25 Appreciate the clear breakdown and context! You're completely right — the only overlap is in the regenerated template typing declarations. If #1099 merges first or if anything changes on main, I'm glad to rebase and re-run the generator on top of current main immediately so everything lands cleanly. |
There was a problem hiding this comment.
This is a sensible, minimal fix: getEmDashCollection/getEmDashEntry already attach entry.data.byline, so the generated emdash-env.d.ts interfaces should expose it. The core change in packages/core/src/schema/zod-generator.ts is limited to adding the byline property and the BylineSummary import, consistent with the existing bylines/terms hydrated fields.
I verified:
hydrateEntryBylinesinquery.tssetsdata.byline = credits[0]?.byline ?? null, so the type matches runtime.BylineSummaryis exported as a type frompackages/core/src/index.ts, so the generated import resolves.- The previously malformed
templates/blog/emdash-env.d.tsis now clean — no duplicatedbylinelines. - All 23 tracked
emdash-env.d.tsfiles include theBylineSummaryimport and a singlebyline?: BylineSummary | null;line per interface. - The
zod-generator.test.tsadditions cover both the generated interface string and the generated file imports.
Remaining notes (non-blocking):
- The changeset body would be stronger with a present-tense verb lead (the changeset README recommends leading with "Fixes", "Adds", etc.).
- Several template
emdash-env.d.tsfiles (e.g.templates/marketing,demos/plugins-demo,packages/core/tests/integration/fixture) pre-date the generator's currentterms?/TaxonomyTermoutput. They are now consistent with their prior state plusbyline, but they are not a fully clean regeneration. Worth a follow-up generator pass, though not required for this bug fix.
| "emdash": patch | ||
| --- | ||
|
|
||
| Generated `emdash-env.d.ts` files now include `entry.data.byline?: BylineSummary | null` alongside `bylines`, matching the byline data hydrated by `getEmDashEntry` and `getEmDashCollection` at runtime. |
There was a problem hiding this comment.
[suggestion] The body is technically accurate, but the changeset README asks changesets to lead with a present-tense verb ("Fixes", "Adds", "Updates", etc.) and name the user-facing surface. Starting with "Generated ..." buries the action.
| Generated `emdash-env.d.ts` files now include `entry.data.byline?: BylineSummary | null` alongside `bylines`, matching the byline data hydrated by `getEmDashEntry` and `getEmDashCollection` at runtime. | |
| Fixes generated `emdash-env.d.ts` files to include `entry.data.byline?: BylineSummary | null` alongside `bylines`, matching the byline data hydrated by `getEmDashEntry` and `getEmDashCollection` at runtime. |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
|
@ascorbic @amasen02 We shoud probably rerun tests Copilot: I'll help you find a solution for this failing job. Let me start by retrieving the job logs.Now let me get the test file to understand what the test is doing:## Solution The test Root Causes Identified:
Recommended FixThe issue is in the In // Add this before packing packages in beforeAll hook:
async function ensurePackagesAreBuilt(packages: WorkspacePackage[]): Promise<void> {
await Promise.all(
packages.map(({ dir }) =>
runPnpm(["run", "build"], dir, 120_000)
),
);
}
// Then in beforeAll, before packLocalPackages:
beforeAll(async () => {
await ensureBuilt();
temporaryDirectory = mkdtempSync(join(tmpdir(), "emdash-isolated-install-"));
const tarballDir = join(temporaryDirectory, "tarballs");
const storeDir = join(temporaryDirectory, "pnpm-store");
mkdirSync(tarballDir);
const workspacePackages = localPackageClosure(discoverWorkspacePackages());
await ensurePackagesAreBuilt(workspacePackages); // Add this line
const tarballs = await packLocalPackages(workspacePackages, tarballDir);
// ... rest of the codeAlternative: Verify package.json includes build outputs in files field Ensure the root {
"files": [
"dist",
"src",
"package.json"
]
}The core issue is that the packed tarballs need to include the compiled output (dist files) so that when installed in the isolated environment, the binary references and Vite virtual modules can be properly resolved. |
3 participants
What does this PR do?
Adds
byline?: BylineSummary | null;to collection interfaces generated byzod-generator.tsand importsBylineSummaryfromemdashin generatedemdash-env.d.tsdeclaration files.At runtime,
hydrateEntryBylinesinquery.tsattaches bothentry.data.bylines(ContentBylineCredit[]) andentry.data.byline(BylineSummary | null), but the type generator previously only emittedbylines?. This enables type-safe access toentry.data.bylinewithout requiring manual casts.Closes #2888
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Not applicable (non-UI change). Targeted unit tests in
zod-generator.test.tspass cleanly: