Local dir is not clean up properly before switching to target branch

[scinos]

We found a scenario where this action is not completely cleaning up local directory before switching to the target branch, resulting in an invalid build:

Let's say I have a master branch that has a single dep, and two packages (one root, one nested):

./package.json
./node_mooules/[email protected]
./packages/some-package/package.json

And then imagine I have a branch that updates lib dependency in my nested package, so npm i generates:

./package.json
./node_mooules/[email protected]
./packages/some-package/package.json
./packages/some-package/node_modules/[email protected]

So far so good. The above structure is valid and works without problems. However, when using this action it will essentially do (the build step is not important):

• Checkout branch
• Run npm ci
• Checkout master
• Run npm ci

Running npm ci cleans ./node_modules, but it does not clean ./packages/some-package/node_modules (tested with npm 6.14.12), so when checking out master there is still a leftover /packages/some-package/node_modules/[email protected] present, and it is never clean up. The resulting combination may break and fail to compile (in fact, it did in WordPress/gutenberg#31476 (comment), where we found the issue).

I think the solution would be to do a manual rm -fr **/node_modules before checking out the target branch.

[developit]

Hiya! npm doesn't specify any behavior for **/node_modules (unless npm 7 workspaces are used, and only for directories matched by the configured workspaces). From poking around the repo, it seems like Gutenberg is using lerna, which is effectively the thing that is orchestrating those nested node_modules.

So, while it would be too arbitrary for compressed-size-action to rm-rf nested node_modules directories between builds (since they can be source code), it is possible to do this yourself using a postinstall script like the one already defined that runs patch-package:

{
  "scripts": {
-    "postinstall": "patch-package && node ./patches/patch-xcode.js",
+    "postinstall": "lerna && patch-package && node ./patches/patch-xcode.js",
  }
}

This would apply to local installations in addition to CI, though. To fix this specifically in CI, you can tell compressed-size-action to run npm run build:ci instead of npm run build:

jobs:
    build:
        name: Check
        runs-on: ubuntu-latest
        steps:
            // <snip>
            - uses: preactjs/compressed-size-action@7d87f60a6b0c7d193b8183ce859ed00b356ea92f # v2.1.0
              with:
                  repo-token: '${{ secrets.GITHUB_TOKEN }}'
                  pattern: '{build/**/*.min.js,build/**/*.css}'
                  build-script: 'build:ci'

... then define build:ci to run the existing distclean script prior to building:

{
  "scripts": {
    "build:ci": "npm run distclean && lerna && npm run build"
  }
}

(I believe the lerna command is required in order to repopulate those nested node_modules directories).

[scinos]

I feels wrong to have a command that destroys and re-creates node_modules (I haven't tested it, but I think it should be "build:ci": "npm run distclean && npm ci..."), whose only use case right now is for this action.

Would you consider adding a configurable clean-script command to the action?

[developit]

@scinos yes - that's a good idea!