Skip to content

Disable caching when publishing built assets #79835

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
desrosj wants to merge 10 commits into
base: trunk
Choose a base branch
from
+12 −3
Open

Disable caching when publishing built assets #79835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b665f1c
Disable caching when publishing built assets.
desrosj Jul 2, 2026
8feb9ce
Refine caching logic for building plugin zips.
desrosj Jul 2, 2026
983b286
Merge branch 'trunk' into actions/disable-caching-for-production-builds
desrosj Jul 9, 2026
d1f5d01
Merge branch 'trunk' into actions/disable-caching-for-production-builds
desrosj Jul 14, 2026
03bd071
Only manually cache when desired.
desrosj Jul 14, 2026
bee5cc9
Disable all cache paths when enable-caching is false
Copilot Jul 14, 2026
27ae035
Flip conditional to check for true.
desrosj Jul 14, 2026
d111a36
Merge branch 'actions/disable-caching-for-production-builds' of https…
desrosj Jul 14, 2026
95eabe8
Remove `cache` input.
desrosj Jul 14, 2026
d36d9b3
Merge branch 'trunk' into actions/disable-caching-for-production-builds
desrosj Jul 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions .github/setup-node/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ inputs:
node-version:
description: 'Optional. The Node.js version to use. When not specified, the version specified in .nvmrc will be used.'
required: false
enable-caching:
description: 'Whether to enable and configure caching for npm.'
required: true
default: 'true'
Comment on lines +9 to +10

@tyxla tyxla Jul 15, 2026

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.

Why do we have a default for a required input?


runs:
using: 'composite'
Expand All @@ -13,8 +17,8 @@ runs:
with:
node-version-file: ${{ inputs.node-version == '' && '.nvmrc' || '' }}
node-version: ${{ inputs.node-version }}
package-manager-cache: ${{ inputs.enable-caching == 'true' && 'true' || 'false' }}

@ciampo ciampo Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Flagged during ai-assisted review

The new guards correctly make the node_modules cache opt-outable, but removing cache: npm also disables actions/setup-node's npm cache for every default caller.

package-manager-cache: true only enables automatic caching when the root package.json declares npm in devEngines.packageManager or the top-level packageManager field (upstream implementation, detection logic). Gutenberg declares neither, so the default enable-caching: true path no longer restores or saves the npm cache across the action's 13 call sites. That contradicts the new input's contract and creates a broad CI performance regression unrelated to the production hardening.

Could we either remove the currently unused composite-action input, or preserve the explicit cache conditionally?

cache: ${{ inputs.enable-caching == 'true' && 'npm' || '' }}
package-manager-cache: false

check-latest: true
Comment thread
desrosj marked this conversation as resolved.
cache: npm

- name: Get Node.js and npm version
id: node-version
Expand All @@ -24,13 +28,14 @@ runs:

- name: Cache node_modules
id: cache-node_modules
if: ${{ inputs.enable-caching == 'true' }}
uses: actions/cache@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0
with:
path: '**/node_modules'
key: node_modules-${{ runner.os }}-${{ runner.arch }}-${{ steps.node-version.outputs.NODE_VERSION }}-${{ hashFiles('package-lock.json', 'patches/**') }}

- name: Install npm dependencies
if: ${{ steps.cache-node_modules.outputs.cache-hit != 'true' }}
if: ${{ inputs.enable-caching == 'false' || steps.cache-node_modules.outputs.cache-hit != 'true' }}

@tyxla tyxla Jul 15, 2026

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.

If caching is disabled the cache step is skipped, so cache-hit != true is already true, no?

run: |
npm ci
shell: bash
Expand All @@ -44,7 +49,7 @@ runs:
# On cache hit, we run the post-install script to match the native `npm ci` behavior.
# An example of this is to patch `node_modules` using patch-package.
- name: Post-install
if: ${{ steps.cache-node_modules.outputs.cache-hit == 'true' }}
if: ${{ inputs.enable-caching == 'true' && steps.cache-node_modules.outputs.cache-hit == 'true' }}
run: |
# Run the post-install script for the root project.
npm run postinstall
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/build-plugin-zip.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ jobs:
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: '.nvmrc'
package-manager-cache: ${{ matrix.IS_GUTENBERG_PLUGIN == 'true' && github.event_name == 'pull_request' && 'true' || 'false' }}

@ciampo ciampo Jul 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Flagged during ai-assisted review

Keeping npm caching for Gutenberg-plugin PR builds makes sense, but this condition never enables it. matrix.IS_GUTENBERG_PLUGIN comes from the YAML booleans [true, false], while the expression compares it with the string 'true'; under GitHub's loose-equality rules, boolean true is coerced to 1 and the non-numeric string is NaN, so the comparison is false. Even without that mismatch, package-manager-cache: true cannot auto-detect npm because this repository has no packageManager declaration.

Could we make the intended cache input explicit instead, or remove the exception if caching should now stay disabled for all plugin builds?

cache: ${{ matrix.IS_GUTENBERG_PLUGIN && github.event_name == 'pull_request' && 'npm' || '' }}
package-manager-cache: false

@tyxla tyxla Jul 15, 2026

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.

Yeah, I'd just drop the string comparison ( == 'true') and keep matrix.IS_GUTENBERG_PLUGIN

Comment thread
ciampo marked this conversation as resolved.
check-latest: true

- name: Configure build for wordpress-develop
Expand Down Expand Up @@ -561,6 +562,7 @@ jobs:
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: 'main/.nvmrc'
package-manager-cache: false
registry-url: 'https://registry.npmjs.org'
check-latest: true

Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/publish-npm-packages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ jobs:
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: 'cli/.nvmrc'
package-manager-cache: false
registry-url: 'https://registry.npmjs.org'
check-latest: true

Expand All @@ -89,6 +90,7 @@ jobs:
uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0
with:
node-version-file: 'publish/.nvmrc'
package-manager-cache: false
registry-url: 'https://registry.npmjs.org'
check-latest: true

Expand Down
Loading