feat(bundler): define BundlerConfig public API#5882
feat(bundler): define BundlerConfig public API#5882killagu wants to merge 1 commit intosplit/10-bundler-generate-manifestfrom
Conversation
Introduce the BundlerConfig type surface that subsequent commits (Bundler orchestration + egg-bin CLI wrapper) will consume. bundle() still throws until the orchestration lands. Also re-export ExternalsResolver and ManifestLoader from the package entry so consumers can reach them directly from '@eggjs/egg-bundler'. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Code Review
This pull request initializes the @eggjs/egg-bundler package by defining its core configuration interfaces and a placeholder bundle function. Feedback includes correcting import and export file extensions to .js for ESM compatibility and fixing a potential typo in the documentation comments regarding the @eggjs/pack dependency.
| export { ExternalsResolver, type ExternalsConfig, type ExternalsResolverOptions } from './lib/ExternalsResolver.ts'; | ||
| export { ManifestLoader, type ManifestLoaderOptions } from './lib/ManifestLoader.ts'; |
There was a problem hiding this comment.
In TypeScript ESM, import and export paths should use the .js extension (or the extension that will exist at runtime) rather than .ts. Using .ts extensions in import paths is non-standard and will cause errors in Node.js and most TypeScript configurations unless specific flags like allowImportingTsExtensions are used. For standard ESM compatibility, it is recommended to use .js extensions in the source code.
| export { ExternalsResolver, type ExternalsConfig, type ExternalsResolverOptions } from './lib/ExternalsResolver.ts'; | |
| export { ManifestLoader, type ManifestLoaderOptions } from './lib/ManifestLoader.ts'; | |
| export { ExternalsResolver, type ExternalsConfig, type ExternalsResolverOptions } from './lib/ExternalsResolver.js'; | |
| export { ManifestLoader, type ManifestLoaderOptions } from './lib/ManifestLoader.js'; |
| readonly mode?: 'production' | 'development'; | ||
| /** External package overrides. */ | ||
| readonly externals?: BundlerExternalsConfig; | ||
| /** @utoo/pack tuning. */ |
There was a problem hiding this comment.
The reference to @utoo/pack appears to be a typo for @eggjs/pack. Using the consistent @eggjs organization prefix is recommended for clarity and correctness, unless @utoo/pack is a specific internal dependency intended for this configuration.
| /** @utoo/pack tuning. */ | |
| /** @eggjs/pack tuning. */ |
| @@ -0,0 +1,9 @@ | |||
| import { describe, expect, it } from 'vitest'; | |||
|
|
|||
| import { bundle } from '../src/index.ts'; | |||
There was a problem hiding this comment.
For consistency with ESM standards, it is recommended to use the .js extension in import paths, even when referencing .ts files. While Vitest handles .ts extensions, using .js is the standard approach for TypeScript ESM to ensure compatibility across different environments.
| import { bundle } from '../src/index.ts'; | |
| import { bundle } from '../src/index.js'; |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Defines the initial public API surface for @eggjs/egg-bundler by exporting configuration/result types and a bundle() entrypoint (currently a placeholder), plus a smoke test asserting the placeholder behavior.
Changes:
- Export
BundlerConfig/related config types andBundleResultfromsrc/index.ts - Add
bundle()public function that currently throws a “not implemented” error - Add a Vitest smoke test validating the placeholder rejection
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tools/egg-bundler/src/index.ts | Introduces the public API exports (types + placeholder bundle() entrypoint). |
| tools/egg-bundler/test/index.test.ts | Adds a smoke test that asserts bundle() currently rejects with a “not implemented” error. |
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import { bundle } from '../src/index.ts'; | ||
|
|
||
| describe('@eggjs/egg-bundler', () => { | ||
| it('bundle() is a placeholder that throws until implemented', async () => { | ||
| await expect(bundle({ baseDir: '/tmp', outputDir: '/tmp/out' })).rejects.toThrow(/not implemented/); |
There was a problem hiding this comment.
The test hard-codes /tmp paths, which is not cross-platform (fails on Windows) and may also cause collisions across parallel test runs. Use os.tmpdir() + fs.mkdtemp() (or a test temp-dir helper) to create a unique temporary directory, and build baseDir/outputDir with path.join().
| import { describe, expect, it } from 'vitest'; | |
| import { bundle } from '../src/index.ts'; | |
| describe('@eggjs/egg-bundler', () => { | |
| it('bundle() is a placeholder that throws until implemented', async () => { | |
| await expect(bundle({ baseDir: '/tmp', outputDir: '/tmp/out' })).rejects.toThrow(/not implemented/); | |
| import { mkdtemp } from 'node:fs/promises'; | |
| import { tmpdir } from 'node:os'; | |
| import * as path from 'node:path'; | |
| import { describe, expect, it } from 'vitest'; | |
| import { bundle } from '../src/index.ts'; | |
| describe('@eggjs/egg-bundler', () => { | |
| it('bundle() is a placeholder that throws until implemented', async () => { | |
| const baseDir = await mkdtemp(path.join(tmpdir(), 'egg-bundler-')); | |
| const outputDir = path.join(baseDir, 'out'); | |
| await expect(bundle({ baseDir, outputDir })).rejects.toThrow(/not implemented/); |
| export interface BundlerConfig { | ||
| /** Application root directory. Required. */ | ||
| readonly baseDir: string; | ||
| /** Output directory for the bundled artifact. Required. */ | ||
| readonly outputDir: string; | ||
| /** Path to manifest.json. Defaults to `<baseDir>/.egg/manifest.json`. */ | ||
| readonly manifestPath?: string; | ||
| /** Framework name or absolute path. Defaults to `'egg'`. */ | ||
| readonly framework?: string; | ||
| /** Build mode. Defaults to `'production'`. */ | ||
| readonly mode?: 'production' | 'development'; | ||
| /** External package overrides. */ | ||
| readonly externals?: BundlerExternalsConfig; | ||
| /** @utoo/pack tuning. */ | ||
| readonly pack?: BundlerPackConfig; | ||
| /** Enable tegg decoratedFile collection. Defaults to `true`. */ | ||
| readonly tegg?: boolean; | ||
| } |
There was a problem hiding this comment.
The JSDoc on BundlerConfig describes runtime defaults/behavior, but bundle() currently always throws, so those defaults are not actually applied yet. To avoid misleading API docs, either (a) add a doc comment on bundle() explicitly stating it is a placeholder that always throws (and that config defaults are provisional), or (b) adjust the config field docs to indicate the defaults are planned (until implementation lands).
| /** Absolute path to the normalized bundled manifest. */ | ||
| readonly manifestPath: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
The JSDoc on BundlerConfig describes runtime defaults/behavior, but bundle() currently always throws, so those defaults are not actually applied yet. To avoid misleading API docs, either (a) add a doc comment on bundle() explicitly stating it is a placeholder that always throws (and that config defaults are provisional), or (b) adjust the config field docs to indicate the defaults are planned (until implementation lands).
| /** | |
| * Placeholder API for the future bundling implementation. | |
| * | |
| * This function is not implemented yet and always throws. | |
| * The defaults described on {@link BundlerConfig} are planned behavior and | |
| * are not currently applied at runtime. | |
| */ |
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| import { bundle } from '../src/index.ts'; | ||
|
|
||
| describe('@eggjs/egg-bundler', () => { |
There was a problem hiding this comment.
This smoke test verifies the placeholder error, but it doesn’t assert the new public API exports beyond bundle() (e.g., that bundle’s signature stays (_config: BundlerConfig) => Promise<BundleResult>). Consider adding a type-level assertion using Vitest’s expectTypeOf(...) (or a tsd-style test) to lock in the exported types/signature as part of defining the public API.
| import { describe, expect, it } from 'vitest'; | |
| import { bundle } from '../src/index.ts'; | |
| describe('@eggjs/egg-bundler', () => { | |
| import { describe, expect, expectTypeOf, it } from 'vitest'; | |
| import { bundle, type BundleResult, type BundlerConfig } from '../src/index.ts'; | |
| describe('@eggjs/egg-bundler', () => { | |
| it('exports bundle() with the expected public API signature', () => { | |
| expectTypeOf(bundle).toEqualTypeOf<(_config: BundlerConfig) => Promise<BundleResult>>(); | |
| }); |
Exports
BundlerConfigtype andbundle()function fromsrc/index.ts.bundle()currently throws 'Not yet implemented' — wired in C8 (Bundler orchestrator). Addstest/index.test.tssmoke test.Part of #5863 split. Tracking: #5871.
🤖 Generated with Claude Code