Skip to content

fix(dal-runtime): package codegen templates#6030

Closed
elrrrrrrr wants to merge 1 commit into
eggjs:nextfrom
elrrrrrrr:codex/dal-runtime-package-templates
Closed

fix(dal-runtime): package codegen templates#6030
elrrrrrrr wants to merge 1 commit into
eggjs:nextfrom
elrrrrrrr:codex/dal-runtime-package-templates

Conversation

@elrrrrrrr

@elrrrrrrr elrrrrrrr commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

What changed

Copy src/templates into dist/templates through the package tsdown configuration.

Why

@eggjs/dal-runtime publishes only dist, but its Nunjucks templates were not copied by the build. A clean consumer install therefore fails DAL generation with template not found and currently requires Chair-bin to copy three files into node_modules during postinstall.

Verification

  • vitest run --project @eggjs/dal-runtime tegg/core/dal-runtime/test/CodeGenerator.test.ts
  • 5 tests passed

Summary by CodeRabbit

  • Bug Fixes

    • Ensured required templates are included in the build output, preventing missing template files at runtime.
  • Tests

    • Added coverage to verify that templates are copied to the expected distribution directory.

Copilot AI review requested due to automatic review settings July 12, 2026 14:35
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The dal-runtime build now copies src/templates to dist/templates. A test imports the build configuration and verifies this copy mapping.

Changes

Template Asset Build

Layer / File(s) Summary
Configure and verify template copying
tegg/core/dal-runtime/tsdown.config.ts, tegg/core/dal-runtime/test/CodeGenerator.test.ts
The build configuration adds the template copy rule, and a test verifies its source and destination paths.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

  • eggjs/egg#5985: Updates CodeGenerator ESM path handling so runtime template loading can use the copied assets.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: packaging DAL runtime codegen templates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@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 configures templates as build assets in tsdown.config.ts by copying them from src/templates to dist/templates. It also adds a corresponding unit test in CodeGenerator.test.ts to verify this configuration. I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.17%. Comparing base (3ddbbe4) to head (e975414).
⚠️ Report is 2 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #6030      +/-   ##
==========================================
+ Coverage   81.98%   82.17%   +0.18%     
==========================================
  Files         678      678              
  Lines       20838    20842       +4     
  Branches     4154     4155       +1     
==========================================
+ Hits        17085    17127      +42     
+ Misses       3235     3207      -28     
+ Partials      518      508      -10     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI 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.

Pull request overview

This PR fixes @eggjs/dal-runtime packaging so its Nunjucks templates are included in published artifacts, preventing “template not found” failures in clean consumer installs.

Changes:

  • Update tsdown build config to copy src/templates into dist/templates.
  • Add a unit test that asserts the build config includes the expected template asset copy rule.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tegg/core/dal-runtime/tsdown.config.ts Copies Nunjucks templates into dist/ so they’re present in the published package.
tegg/core/dal-runtime/test/CodeGenerator.test.ts Adds a test to lock in the tsdown asset-copy configuration for templates.

Comment on lines +18 to +26
it('should configure templates as build assets', async () => {
const config = (await import('../tsdown.config.ts')).default as any;
assert.deepEqual(config.copy, [
{
from: 'src/templates',
to: 'dist/templates',
},
]);
});

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tegg/core/dal-runtime/test/CodeGenerator.test.ts (1)

18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Verify the emitted artifact, not only the config object.

This test can pass even when the build places files under the wrong nested directory. Add an integration assertion that runs the package build in a temporary output directory and verifies a representative template exists at dist/templates/....

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tegg/core/dal-runtime/test/CodeGenerator.test.ts` around lines 18 - 26,
Extend the test “should configure templates as build assets” to run the package
build with a temporary output directory, then assert that a representative file
from src/templates is emitted under dist/templates/... . Keep the existing
config.copy assertion, and clean up the temporary build output after the
integration check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tegg/core/dal-runtime/tsdown.config.ts`:
- Around line 7-12: Update the copy configuration so the `src/templates`
contents are placed directly under the output directory’s `templates` path; in
the `copy` entry, omit `to` or set it to `dist` rather than `dist/templates`,
ensuring `CodeGenerator` can find templates at `dist/templates`.

---

Nitpick comments:
In `@tegg/core/dal-runtime/test/CodeGenerator.test.ts`:
- Around line 18-26: Extend the test “should configure templates as build
assets” to run the package build with a temporary output directory, then assert
that a representative file from src/templates is emitted under
dist/templates/... . Keep the existing config.copy assertion, and clean up the
temporary build output after the integration check.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 35180647-b249-4d44-8dd5-71e588402887

📥 Commits

Reviewing files that changed from the base of the PR and between dfd102c and e975414.

📒 Files selected for processing (2)
  • tegg/core/dal-runtime/test/CodeGenerator.test.ts
  • tegg/core/dal-runtime/tsdown.config.ts

Comment on lines +7 to +12
copy: [
{
from: 'src/templates',
to: 'dist/templates',
},
],

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

rg -n '"tsdown"|"outDir"|from:|to:' \
  --glob 'package.json' --glob '*lock*' --glob 'tsdown.config.ts' .

Repository: eggjs/egg

Length of output: 1908


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant config and surrounding package settings.
cat -n tegg/core/dal-runtime/tsdown.config.ts
printf '\n--- package.json ---\n'
cat -n tegg/core/dal-runtime/package.json 2>/dev/null || true
printf '\n--- root package.json ---\n'
cat -n package.json | sed -n '1,120p'

# Look for repo-local tsdown copy-path examples and any notes about outDir/copy behavior.
printf '\n--- tsdown configs in repo ---\n'
rg -n "copy:\s*\[|from:\s*'src/templates'|to:\s*'dist/templates'|outDir" \
  tegg tools packages examples --glob 'tsdown.config.ts' --glob 'package.json'

Repository: eggjs/egg

Length of output: 7495


🌐 Web query:

tsdown copy entry to destination basename appends source basename docs Interface.CopyEntry

💡 Result:

In tsdown, the CopyEntry interface is used to define rules for copying files or directories from a source location to a destination during the build process [1][2][3]. The behavior regarding source and destination basenames depends on how the entry is configured [2][3]: 1. Default Behavior: When providing a string for a copy entry, tsdown defaults to placing the file or directory into the output directory (outDir) using its original basename [4]. 2. CopyEntry Object: By using an object with the CopyEntry interface, you can explicitly define the destination path using the to property [2][3]. - If the flatten option is set to true (which is the default), files are copied directly into the destination folder, effectively stripping their original directory structure and using the source basename as the file name [2][3]. - If you need to transform the destination name, the rename property within CopyEntry allows you to provide a string or a function to modify the destination file or folder name [2][3]. The internal implementation uses the resolveCopyEntry function, which resolves the destination path by joining the destination folder with the base name (or the renamed target) of the source file [2][3]. For full technical details, you can refer to the official API documentation for Interface: CopyEntry [1].

Citations:


Use the output directory as the copy destination base. from: 'src/templates' with to: 'dist/templates' resolves to dist/templates/templates, so CodeGenerator won’t find templates at dist/templates in a clean install. Omit to or set it to dist.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tegg/core/dal-runtime/tsdown.config.ts` around lines 7 - 12, Update the copy
configuration so the `src/templates` contents are placed directly under the
output directory’s `templates` path; in the `copy` entry, omit `to` or set it to
`dist` rather than `dist/templates`, ensuring `CodeGenerator` can find templates
at `dist/templates`.

@elrrrrrrr

Copy link
Copy Markdown
Contributor Author

Superseded by #6033. This patch is already included in the beta.24 baseline branch (codex/release-beta24-patchless) and was published with v4.1.2-beta.24.

@elrrrrrrr elrrrrrrr closed this Jul 12, 2026
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.

2 participants