Create abstract client steps infrastracture to externalize the ap modules client configurations #6965
+239
−8
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import {executeStep, BuildContext, LifecycleStep} from './client-steps.js' | ||
| import * as stepsIndex from './steps/index.js' | ||
| import {ExtensionInstance} from '../../models/extensions/extension-instance.js' | ||
| import {beforeEach, describe, expect, test, vi} from 'vitest' | ||
|
|
||
| vi.mock('./steps/index.js') | ||
|
|
||
| describe('executeStep', () => { | ||
| let mockContext: BuildContext | ||
|
|
||
| beforeEach(() => { | ||
| mockContext = { | ||
| extension: { | ||
| directory: '/test/dir', | ||
| outputPath: '/test/output/index.js', | ||
| } as ExtensionInstance, | ||
| options: { | ||
| stdout: {write: vi.fn()} as any, | ||
| stderr: {write: vi.fn()} as any, | ||
| app: {} as any, | ||
| environment: 'production' as const, | ||
| }, | ||
| stepResults: new Map(), | ||
| } | ||
| }) | ||
|
|
||
| const step: LifecycleStep = { | ||
| id: 'test-step', | ||
| name: 'Test Step', | ||
| type: 'include_assets', | ||
| config: {}, | ||
| } | ||
|
|
||
| describe('success', () => { | ||
| test('returns a successful StepResult with output', async () => { | ||
| vi.mocked(stepsIndex.executeStepByType).mockResolvedValue({filesCopied: 3}) | ||
|
|
||
| const result = await executeStep(step, mockContext) | ||
|
|
||
| expect(result.id).toBe('test-step') | ||
| expect(result.success).toBe(true) | ||
| if (result.success) expect(result.output).toEqual({filesCopied: 3}) | ||
| expect(result.duration).toBeGreaterThanOrEqual(0) | ||
| }) | ||
|
|
||
| test('logs step execution to stdout', async () => { | ||
| vi.mocked(stepsIndex.executeStepByType).mockResolvedValue({}) | ||
|
|
||
| await executeStep(step, mockContext) | ||
|
|
||
| expect(mockContext.options.stdout.write).toHaveBeenCalledWith('Executing step: Test Step\n') | ||
| }) | ||
| }) | ||
|
|
||
| describe('failure', () => { | ||
| test('throws a wrapped error when the step fails', async () => { | ||
| vi.mocked(stepsIndex.executeStepByType).mockRejectedValue(new Error('something went wrong')) | ||
|
|
||
| await expect(executeStep(step, mockContext)).rejects.toThrow( | ||
| 'Build step "Test Step" failed: something went wrong', | ||
| ) | ||
| }) | ||
|
|
||
| test('returns a failure result and logs a warning when continueOnError is true', async () => { | ||
| vi.mocked(stepsIndex.executeStepByType).mockRejectedValue(new Error('something went wrong')) | ||
|
|
||
| const result = await executeStep({...step, continueOnError: true}, mockContext) | ||
|
|
||
| expect(result.success).toBe(false) | ||
| if (!result.success) expect(result.error?.message).toBe('something went wrong') | ||
| expect(mockContext.options.stderr.write).toHaveBeenCalledWith( | ||
| 'Warning: Step "Test Step" failed but continuing: something went wrong\n', | ||
| ) | ||
| }) | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| import {executeStepByType} from './steps/index.js' | ||
| import type {ExtensionInstance} from '../../models/extensions/extension-instance.js' | ||
| import type {ExtensionBuildOptions} from './extension.js' | ||
|
|
||
| /** | ||
| * LifecycleStep represents a single step in the client-side build pipeline. | ||
| * Pure configuration object — execution logic is separate (router pattern). | ||
| */ | ||
| export interface LifecycleStep { | ||
| /** Unique identifier, used as the key in the stepResults map */ | ||
| readonly id: string | ||
|
|
||
| /** Human-readable name for logging */ | ||
| readonly name: string | ||
|
|
||
| /** Step type (determines which executor handles it) */ | ||
| readonly type: | ||
| | 'include_assets' | ||
| | 'build_theme' | ||
| | 'bundle_theme' | ||
| | 'bundle_ui' | ||
| | 'copy_static_assets' | ||
| | 'build_function' | ||
| | 'create_tax_stub' | ||
| | 'esbuild' | ||
| | 'validate' | ||
| | 'transform' | ||
| | 'custom' | ||
|
|
||
| /** Step-specific configuration */ | ||
| readonly config: {[key: string]: unknown} | ||
|
|
||
| /** Whether to continue on error (default: false) */ | ||
| readonly continueOnError?: boolean | ||
| } | ||
|
|
||
| /** | ||
| * A group of steps scoped to a specific lifecycle phase. | ||
| * Allows executing only the steps relevant to a given lifecycle (e.g. 'deploy'). | ||
| */ | ||
| interface ClientLifecycleGroup { | ||
| readonly lifecycle: 'deploy' | ||
| readonly steps: ReadonlyArray<LifecycleStep> | ||
| } | ||
|
|
||
| /** | ||
| * The full client steps configuration for an extension. | ||
| * Replaces the old buildConfig contract. | ||
| */ | ||
| export type ClientSteps = ReadonlyArray<ClientLifecycleGroup> | ||
alfonso-noriega marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Context passed through the step pipeline. | ||
| * Each step can read from and write to the context. | ||
| */ | ||
| export interface BuildContext { | ||
|
Contributor
There was a problem hiding this comment. i'm unclear on why an additional stateful context is necessary. can we not iterate through the steps just using the data already available and the step i/o? |
||
| readonly extension: ExtensionInstance | ||
| readonly options: ExtensionBuildOptions | ||
| readonly stepResults: Map<string, StepResult> | ||
| [key: string]: unknown | ||
|
Contributor
There was a problem hiding this comment. ideally outputs are not unknown if there is a finite amount of valid steps |
||
| } | ||
|
|
||
| type StepResult = { | ||
| readonly id: string | ||
| readonly duration: number | ||
| } & ( | ||
| | { | ||
| readonly success: false | ||
| readonly error: Error | ||
| } | ||
| | { | ||
| readonly success: true | ||
| readonly output: unknown | ||
| } | ||
| ) | ||
|
|
||
| /** | ||
| * Executes a single client step with error handling. | ||
| */ | ||
| export async function executeStep(step: LifecycleStep, context: BuildContext): Promise<StepResult> { | ||
| const startTime = Date.now() | ||
|
|
||
| try { | ||
| context.options.stdout.write(`Executing step: ${step.name}\n`) | ||
| const output = await executeStepByType(step, context) | ||
|
|
||
| return { | ||
| id: step.id, | ||
| success: true, | ||
| duration: Date.now() - startTime, | ||
| output, | ||
| } | ||
|
There was a problem hiding this comment.
|
||
| } catch (error) { | ||
| const stepError = error as Error | ||
|
|
||
| if (step.continueOnError) { | ||
| context.options.stderr.write(`Warning: Step "${step.name}" failed but continuing: ${stepError.message}\n`) | ||
| return { | ||
| id: step.id, | ||
| success: false, | ||
| duration: Date.now() - startTime, | ||
| error: stepError, | ||
| } | ||
| } | ||
|
|
||
| throw new Error(`Build step "${step.name}" failed: ${stepError.message}`) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import type {LifecycleStep, BuildContext} from '../client-steps.js' | ||
|
|
||
| /** | ||
| * Routes step execution to the appropriate handler based on step type. | ||
| * This implements the Command Pattern router, dispatching to type-specific executors. | ||
| * | ||
| * @param step - The build step configuration | ||
| * @param context - The build context | ||
| * @returns The output from the step execution | ||
| * @throws Error if the step type is not implemented or unknown | ||
| */ | ||
| export async function executeStepByType(step: LifecycleStep, _context: BuildContext): Promise<unknown> { | ||
| switch (step.type) { | ||
| // Future step types (not implemented yet): | ||
| case 'include_assets': | ||
| case 'build_theme': | ||
| case 'bundle_theme': | ||
| case 'bundle_ui': | ||
| case 'copy_static_assets': | ||
| case 'build_function': | ||
| case 'create_tax_stub': | ||
| case 'esbuild': | ||
| case 'validate': | ||
| case 'transform': | ||
| case 'custom': | ||
| throw new Error(`Build step type "${step.type}" is not yet implemented.`) | ||
|
There was a problem hiding this comment. Step router makes all step types unusable (always throws)
Evidence: case 'include_assets':
...
throw new Error(`Build step type "${step.type}" is not yet implemented.`)Impact:
|
||
|
|
||
| default: | ||
| throw new Error(`Unknown build step type: ${(step as {type: string}).type}`) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
clientStepsmeant to eventually replacebuildconfigor are we intending to support both?