Skip to content

Commit

Permalink
[build-tools] Default missing output values to empty string (#500)
Browse files Browse the repository at this point in the history
# Why

https://exponent-internal.slack.com/archives/C06EFBQK3B7/p1737419576613249

# How

When we're collecting job outputs from steps, we treat values as strings which may include template expressions. So:
```yaml
outputs:
  foo: bar # sets foo="bar"
  build_id: ${{ steps.find_build.outputs.build_id }} # acts how `${steps.find_build.outputs.build_id}` in JS would act
  key: ios-${{ ... }} # concatenates "ios-" and whatever template expression evaluates to
  counter: ${{ 2 + 3 }} # sets counter="5"
```

I see a way of reasoning about `outputs` that they are never expected to be used as static values, i.e. `foo: bar` and `key: ios-${{ ... }}` are never what the user will want to use and instead we should always use value as an expression. However,
```yaml
outputs:
  foo: "\"bar\""
  build_id: steps.find_build.outputs.build_id
  key: "\"ios-\" + ..."
  counter: 2 + 3
```
looks strange to me.

We could also not stringify results and do what James suggests, let `undefined` be `undefined`. However, that would also mean `2` is `2`, not `"2"`. And I think it would be better for users that _do_ want this level of type-correctness to explicitly do `counter: ${{ toJSON(2) }}` and then `fromJSON(needs.setup.outputs.counter)`.

# Test Plan

Added test.
  • Loading branch information
sjchmiela authored Jan 22, 2025
1 parent 4d9a65a commit 6920367
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
13 changes: 13 additions & 0 deletions packages/build-tools/src/utils/__tests__/outputs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,19 @@ describe(getJobOutputsFromSteps, () => {
})
).toEqual({ fingerprint_hash: 'abc' });
});

it('defaults missing values to empty string', () => {
expect(
getJobOutputsFromSteps({
jobOutputDefinitions: {
fingerprint_hash: '${{ steps.setup.outputs.fingerprint_hash }}',
},
interpolationContext: {
steps: { setup: { outputs: {} } },
},
})
).toEqual({ fingerprint_hash: '' });
});
});

describe(uploadJobOutputsToWwwAsync, () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/build-tools/src/utils/outputs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export function getJobOutputsFromSteps({
const jobOutputs: Record<string, string | undefined> = {};
for (const [outputKey, outputDefinition] of Object.entries(jobOutputDefinitions)) {
const outputValue = outputDefinition.replace(/\$\{\{(.+?)\}\}/g, (_match, expression) => {
return `${jsepEval(expression, interpolationContext)}`;
return `${jsepEval(expression, interpolationContext) ?? ''}`;
});

jobOutputs[outputKey] = outputValue;
Expand Down

0 comments on commit 6920367

Please sign in to comment.