core.setOutput should(?) transform data to a string

[JasonEtco]

Describe the enhancement

Hello! In an action I maintain, I call setOutput with an issue number. That fails, because setOutput expects it to be a string. This bit me (JasonEtco/create-an-issue#46), and I'm wondering if y'all would be up for transforming the value passed to a string, or throwing if it isn't a string.

I realize that this gets into "just use TypeScript" territory, but I see no reason that I shouldn't be able to pass a number aside from the implementation detail of how it works under the hood.

Code Snippet

// Doesn't work but could
core.setOutput('issue_number', 1)

// Does work
core.setOutput('issue_number', '1')
core.setOutput('issue_number', String(1))

Additional information

I'm using JasonEtco/actions-toolkit as an opinionated alternative/wrapper for actions/toolkit. If transforming the value isn't something y'all feel good about, I'm happy to move that into that more opinionated territory!

[ericsciple]

@thboop thoughts?

I think this makes sense. Especially given common cases like bool/number.

The only thing that worries me a little bit, is if we ever preserve type/structure in the future. Basically to enable something like ${{ steps.foo.outputs.bar.subproperty }}. However we could always overload in the future... Also it's nice to just keep it simple (outputs are strings), so i think converting to string makes sense.

I wonder for arrays/objects, whether we should convert to json (indented?) or just defer to however nodejs converts to string - for example:

`${[1, 2, 3]}` // '1,2,3'

i kind of like just doing whatever nodejs does (no magic)

[JasonEtco]

Thanks for sharing those thoughts @ericsciple, there are a couple of complications you mentioned.

I wonder for arrays/objects, whether we should convert to json (indented?)

This is an interesting choice; using the String primitive on an object, it does this:

String({ test: true })
// -> "[object Object]"

Which is not at all useful. This is way better:

JSON.stringify({ test: true })
// -> '{ "test": true }'

JSON.stringify can also be applied numbers, arrays an booleans - so that might be a nice catch-all approach.

[thboop]

I think stringifying makes a lot of sense. Bools and numbers are probably the most common case here, but we should consider others (like json).

Also it's nice to just keep it simple (outputs are strings), so i think converting to string makes sense. Agreed.

[JasonEtco]

One thing to note is that there may be an expectation that whatever you setOutput will be the same type on the other end - that sounds like something for docs (and obviously TypeScript).

[ericsciple]

+1 i would just write in the function description that it runs JSON.stringify for any values that arent a string

some quirks to be aware of :)

JSON.stringify(null) // 'null'
JSON.stringify(undefined) // undefined - so need to explicitly handle undefined

[jetersen]

Fell into this trap: release-drafter/release-drafter#376 (comment)

[thboop]

Resolved in #405