Clean up module outputFile calculation and move it to the specifications#6967
Conversation
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
|
🤖 Code Review · #projects-dev-ai for questions ✅ Complete - No issues 📋 History✅ No issues → ✅ 2 findings → ❌ Failed → ✅ No issues |
alfonso-noriega
changed the base branch from
03-10-create_abstract_client_steps_infrastracture_to_externalize_the_ap_modules_client_configurations
to
graphite-base/6967
alfonso-noriega
force-pushed
the
graphite-base/6967
branch
from
59f0139 to
7b9999e
Compare
alfonso-noriega
force-pushed
the
03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications
branch
from
bbc0817 to
7b68eb7
Compare
Coverage report
Test suite run success3791 tests passing in 1448 suites. Report generated by 🧪jest coverage report action from ecf7d0f |
alfonso-noriega
force-pushed
the
03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications
branch
from
7b68eb7 to
c3ee349
Compare
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
| schema: CheckoutSchema, | ||
| appModuleFeatures: (_) => ['ui_preview', 'cart_url', 'esbuild', 'single_js_entry_path', 'generates_source_maps'], | ||
| buildConfig: {mode: 'ui'}, | ||
| getOutputFileName: (extension: ExtensionInstance<CheckoutConfigType>) => joinPath('dist', `${extension.handle}.js`), |
There was a problem hiding this comment.
dist/dist/... path regression for UI extensions due to double dist/ prefix
getOutputFileName returns joinPath('dist', ${extension.handle}.js), which already includes dist/, while ExtensionInstance builds outputPath as joinPath(this.directory, 'dist', this.outputFileName ?? ''), producing .../<ext>/dist/dist/<handle>.js. This breaks expected artifact locations and later steps (upload/deploy/preview) that assume dist/<handle>.js.
| schema: FunctionExtensionSchema, | ||
| appModuleFeatures: (_) => ['function'], | ||
| buildConfig: {mode: 'function'}, | ||
| getOutputFileName: (_extension: ExtensionInstance<FunctionConfigType>) => joinPath('dist', 'index.wasm'), |
There was a problem hiding this comment.
Function spec returns dist/index.wasm, conflicting with existing function output path logic
Function getOutputFileName returns joinPath('dist', 'index.wasm'), but ExtensionInstance already has function-specific logic that expects the build path to include dist/index.wasm via config/defaultPath, and other code paths use this.outputFileName ?? '' directly. This can lead to inconsistent read/write locations and missing wasm artifacts depending on which path computation is used.
alfonso-noriega
force-pushed
the
03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications
branch
from
c3ee349 to
24fb214
Compare
alfonso-noriega
force-pushed
the
03-10-clean_up_module_outputfile_calculation_and_move_it_to_the_specifications
branch
from
24fb214 to
ecf7d0f
Compare
| } else { | ||
| return `${this.handle}.js` | ||
| } | ||
| return basename(this.specification.getOutputFileName?.(this) ?? '') |
There was a problem hiding this comment.
Do all extension instances always have a build/output file name? extension-instance feels very overloaded today, and I worry we're overloading it further with things that might be optional properties.
There was a problem hiding this comment.
I think the change in this PR actually cleans this. Move the optional implementation to the specification, so each module can decide if it has an outputFileName or not.
This function already existed in extension-instance, but isntead of hardcoded if/else we move the ownership to each specification
3 participants
WHY are these changes introduced?
The
outputFileNameproperty was previously calculated dynamically using a getter method that relied on the extension's build configuration mode. This approach was inflexible and didn't allow for extension-specific customization of output file names.WHAT is this pull request doing?
outputFileNamefrom a computed getter to a direct property onExtensionInstancegetOutputFileNamemethod to theExtensionSpecificationinterfacegetOutputFileNamein all UI extension specifications to return${extension.handle}.jsgetOutputFileNamein function extensions to returnindex.wasmBuildConfigtype to use a discriminated union for better type safetyoutputFileNamein the constructor using the specification's method or defaults to empty stringHow to test your changes?
Measuring impact
How do we know this change was effective? Please choose one:
Checklist