Skip to content

feat: add comment-body output#148

Open
rzzf wants to merge 1 commit into
preactjs:masterfrom
rzzf:feat/add-comment-body-outputs
Open

feat: add comment-body output#148
rzzf wants to merge 1 commit into
preactjs:masterfrom
rzzf:feat/add-comment-body-outputs

Conversation

@rzzf
Copy link
Copy Markdown

@rzzf rzzf commented May 11, 2026

Releted: #2 (comment)

Write the results to outputs so downstream workflows can manage comments on their own. For example, the results can be uploaded as artifacts, and a workflow_run with write permissions can then post comments on PRs from forks.

Compressed Size Comment Body
name: Compressed Size Comment Body

on:
  pull_request:

jobs:
  size:
    runs-on: ubuntu-latest

    steps:
      - id: compressed-size
        uses: preactjs/compressed-size-action@v2
        with:
          repo-token: ${{ secrets.GITHUB_TOKEN }}

      - name: Save comment body
        env:
          COMMENT_BODY: ${{ steps.compressed-size.outputs.comment-body }}
        run: printf '%s\n' "$COMMENT_BODY" > compressed-size-comment.md

      - uses: actions/upload-artifact@v4
        with:
          name: compressed-size-comment
          path: compressed-size-comment.md
Compressed Size Comment Post
name: Compressed Size Comment Post

on:
  workflow_run:
    workflows: ['Compressed Size Comment Body']
    types: [completed]

jobs:
  comment:
    runs-on: ubuntu-latest
    permissions:
      pull-requests: write

    steps:
      - uses: dawidd6/action-download-artifact@v21
        with:
          workflow: ${{ github.event.workflow.id }}
          run_id: ${{ github.event.workflow_run.id }}
          name: compressed-size-comment

      - id: comment
        run: |
          {
            echo 'body<<EOF'
            cat compressed-size-comment.md
            echo EOF
          } >> "$GITHUB_OUTPUT"

      - uses: actions-cool/maintain-one-comment@v3.3.0
        with:
          body: ${{ steps.comment.outputs.body }}
          body-include: '<!-- compressed-size-action -->'
          number: ${{ github.event.pull_request.number }}

@rzzf rzzf force-pushed the feat/add-comment-body-outputs branch from 23a59fe to 7f0947e Compare May 11, 2026 11:05
Comment thread package.json Outdated
@rschristian
Copy link
Copy Markdown
Member

This is what I was concerned with by bumping the actions version:


Size Change: +132 kB (+325.89%) 🆘

Total Size: 173 kB

📦 View Changed
Filename Size Change
index.js 173 kB +132 kB (+325.89%) 🆘

compressed-size-action


Ideally want to avoid such a blow up, even for something that just runs as an action. At the very least, much harder to audit.

@rzzf
Copy link
Copy Markdown
Author

rzzf commented May 12, 2026

This size increase is caused by the circular dependencies in @actions/core and its unfriendly tree-shaking behavior.

I tested this locally with Github Local Action + Docker. If we don’t upgrade to 1.10.0 or later, we won’t be able to use outputs in the current CI environment unless we implement a simple setOutput ourselves. (Maybe my testing approach was incorrect? I’m not entirely sure either. 😅)

Test Workflow
name: Test comment-body

on:
  pull_request:
  push:

permissions:
  contents: read
  issues: write
  pull-requests: write

jobs:
  test:
    name: job
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v5
        with:
          fetch-depth: 0

      - id: compressed-size
        uses: ./
        with:
          repo-token: ${{ secrets.GITHUB_TOKEN }}
          base-ref: master
          pattern: index.js
          comment-key: output-smoke

      - name: Print comment-body output
        env:
          COMMENT_BODY: ${{ steps.compressed-size.outputs.comment-body }}
        run: |
          if [ -z "$COMMENT_BODY" ]; then
            echo "comment-body output is empty" >&2
            exit 1
          fi

          printf '%s\n' "$COMMENT_BODY"
image

If bundle size is a concern, I’d prefer to implement a lightweight setOutput ourselves. WDYT?

For example

export function setOutput(name, value) {
	const outputPath = process.env.GITHUB_OUTPUT;

	if (outputPath) {
		const delimiter = `ghadelimiter_${Date.now()}`;
		fs.appendFileSync(outputPath, `${name}<<${delimiter}${EOL}${value}${EOL}${delimiter}${EOL}`);
		return;
	}

	console.log(
		`::set-output name=${name}::${String(value).replace(/\n/g, '%0A').replace(/\r/g, '%0D')}`
	);
}

@rschristian
Copy link
Copy Markdown
Member

Circular dependencies just result in (some) tools throwing a warning, it has no bearing whatsoever on final bundle size. Treeshaking is also less of an issue compared to plain bundle bloat that's been allowed to occur over there but 🤷

You can try pinning 1.10.0, tiny chance that's better than 1.11.1 (what you have now), but otherwise yes, I'd prefer we implement matching what you dug up earlier. I assume the code you've gathered above is an extracted version of what the lib otherwise uses?

@rzzf
Copy link
Copy Markdown
Author

rzzf commented May 12, 2026

Circular dependencies just result in (some) tools throwing a warning, it has no bearing whatsoever on final bundle size. Treeshaking is also less of an issue compared to plain bundle bloat that's been allowed to occur over there but 🤷

It's nice to know this 👍

I assume the code you've gathered above is an extracted version of what the lib otherwise uses?

Yes, it is based on the implementation from core, with some simplified handling for name and value edge cases.

https://github.com/actions/toolkit/blob/8066c8132268398c77366dae936dac12c03d39ee/packages/core/src/core.ts#L217-L225

@rzzf rzzf force-pushed the feat/add-comment-body-outputs branch from 7f0947e to b1bc5fe Compare May 12, 2026 03:33
@rzzf rzzf force-pushed the feat/add-comment-body-outputs branch from b1bc5fe to 78ecac6 Compare May 12, 2026 03:34
@rzzf

This comment was marked as resolved.

@rschristian
Copy link
Copy Markdown
Member

Any progress will be reported here.

It's on my todo list, I'll get to it when I can. Thanks for bearing with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants