preactjs / compressed-size-action

GitHub Action that adds compressed size changes to your PRs.
https://github.com/marketplace/actions/compressed-size-action
MIT License
602 stars 84 forks source link

Add `install-script` to available inputs. #9

Closed ncphillips closed 4 years ago

ncphillips commented 4 years ago

Purpose

This action now accepts an install-script as an input.

Background

In trying to setup this action for TinaCMS we found it would not work. The problem is that tinacms is a monorepo that uses bootstrap. We need to run npm ci and then npm run bootstrap in order for the packages to build.

Example

name: Compressed Size

on: [pull_request]

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v1
      - uses: preactjs/compressed-size-action@master
        with:
          repo-token: '${{ secrets.GITHUB_TOKEN }}'
          install-script: 'npm run ci'
developit commented 4 years ago

Hiya! This is a little bit misleading because the script passed is not actually executed in bash, so it can't contain much other than a single npm-script to run (things like && are not supported).

I'm wondering if there's a way to support your use-case (which I agree is probably a common one) without requiring a custom install script.

Here's my thought: what if your root package.json had a postinstall script?

{
  "scripts": {
    "ci": "<your current `npm run ci` script here>",
    "postinstall": "npm run -s ci"
  }
}

@ncphillips Also just a note - you'll want to use actions/checkout@v2, not v1.

ncphillips commented 4 years ago

Yeah. Post install script was the better solution to my problem.

developit commented 4 years ago

Awesome! Also, another option would be to use the existing build-script option:

name: Compressed Size

on: [pull_request]

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: preactjs/compressed-size-action@master
        with:
          repo-token: '${{ secrets.GITHUB_TOKEN }}'
          build-script: 'ci'
developit commented 4 years ago

I'm going to make a note about this trick in the readme, thanks for helping hash this out!

ncphillips commented 4 years ago

Sweet 👍 It would also be good to document some more of the inputs. Reading through the code I found that this was way more powerful then it seems.

Some more unsolicited feedback:

I like this package but it's really slow. IMO the ideal would be to build the branches on push, do the measurements, and store the artifacts somehow. Then the PR action wouldn't have to run build twice, it would only run it on the current branch and compare it against the targets' artifact.

Even better would be if I could use this in tandem with my build process. I'd rather tack this script at the end of my usual install, build, test, lint script

edit: I can make issues for those if you'd like, or if you think it's out of scope that's cool too