Skip to content

feat: add comment-body output#148

Merged
rschristian merged 1 commit into
preactjs:masterfrom
rzzf:feat/add-comment-body-outputs
Jun 7, 2026
Merged

feat: add comment-body output#148
rschristian merged 1 commit into
preactjs:masterfrom
rzzf:feat/add-comment-body-outputs

Conversation

@rzzf

@rzzf rzzf commented May 11, 2026

Copy link
Copy Markdown
Contributor

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

rzzf commented May 12, 2026

Copy link
Copy Markdown
Contributor Author

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

rzzf commented May 12, 2026

Copy link
Copy Markdown
Contributor Author

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

rzzf commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

@rschristian Sorry to bother you, but may I ask if there has been any progress on this? 🙏

@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.

@rschristian rschristian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super sorry for the delay, life's just been incredibly hectic for me in the past month or so. I appreciate your work though!

I'll try to cut a release in the next day or so, ideally we'd add something to the ReadMe mentioning this new feature as well but I don't want to block this PR on it as I've already delayed it enough.

Thanks for the patience!

@rschristian rschristian merged commit 55bc264 into preactjs:master Jun 7, 2026
1 check passed
@rzzf

rzzf commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for your time, and thank you for merging this.

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