Allow building with Yarn 2+#322
Conversation
Summary by CodeRabbit
WalkthroughThe changes rename the Yarn build script environment variable from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/frontend-build.sh (1)
74-76: Rename is consistent, but is a breaking change for external callers.Both the default setter (Lines 74–76) and the
docker run -eforwarding (Line 168) are correctly updated. Note that callers of this script that previously setYARN_BUILD_SCRIPTin their pr_check / pipeline env will stop taking effect silently — the defaultbuild:prodwill be used instead. Please call this out in release notes / CHANGELOG or add a short deprecation shim here mirroring the one suggested inuniversal_build.sh, e.g.:Optional deprecation shim
+if [ -z "${APP_YARN_BUILD_SCRIPT:-}" ] && [ -n "${YARN_BUILD_SCRIPT:-}" ]; then + echo "WARNING: YARN_BUILD_SCRIPT is deprecated; use APP_YARN_BUILD_SCRIPT instead." >&2 + export APP_YARN_BUILD_SCRIPT="$YARN_BUILD_SCRIPT" +fi if [ -z "$APP_YARN_BUILD_SCRIPT" ]; then export APP_YARN_BUILD_SCRIPT="build:prod" fiAlso applies to: 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend-build.sh` around lines 74 - 76, The rename of YARN_BUILD_SCRIPT to APP_YARN_BUILD_SCRIPT is breaking for external callers; add a deprecation shim to preserve backward compatibility by checking if YARN_BUILD_SCRIPT is set and, if so, set APP_YARN_BUILD_SCRIPT from it (and log or echo a deprecation warning), while keeping the new default export for APP_YARN_BUILD_SCRIPT; also add a brief note to release notes/CHANGELOG calling out the rename and the temporary shim (reference symbols: APP_YARN_BUILD_SCRIPT and YARN_BUILD_SCRIPT, and the docker run -e forwarding change).universal_build.sh (1)
69-75: Rename looks correct; consider noting the breaking change.The logic change is straightforward and matches the Dockerfile/
src/frontend-build.shcontract (defaultbuild:prodwhen unset). However, this is a breaking rename for any downstream consumer that currently setsYARN_BUILD_SCRIPTin their CI/build environment — they will silently fall back toyarn build:prodwithout any warning.Options to consider:
- Document the rename prominently in the release notes / README.
- Or temporarily honor
YARN_BUILD_SCRIPTas a fallback with a deprecation warning, e.g.:Optional backward-compat shim
elif [[ "$USES_YARN" == true ]]; then # If APP_YARN_BUILD_SCRIPT env var is set use that # Otherwise just build + if [[ -z "${APP_YARN_BUILD_SCRIPT:-}" && -n "${YARN_BUILD_SCRIPT:-}" ]]; then + echo "WARNING: YARN_BUILD_SCRIPT is deprecated (Yarn 2+ treats YARN_* as config keys); use APP_YARN_BUILD_SCRIPT instead." >&2 + APP_YARN_BUILD_SCRIPT="$YARN_BUILD_SCRIPT" + fi if [[ -n "$APP_YARN_BUILD_SCRIPT" ]]; then yarn "$APP_YARN_BUILD_SCRIPT" else yarn build:prod fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@universal_build.sh` around lines 69 - 75, The change renames YARN_BUILD_SCRIPT to APP_YARN_BUILD_SCRIPT which breaks consumers still setting the old var; update the shell logic so that when APP_YARN_BUILD_SCRIPT is unset but YARN_BUILD_SCRIPT is set, the script uses YARN_BUILD_SCRIPT and emits a clear deprecation warning to stderr (e.g., echo >&2) advising to switch to APP_YARN_BUILD_SCRIPT, otherwise default to running yarn build:prod; target the if-block that checks APP_YARN_BUILD_SCRIPT and add the fallback/deprecation handling for YARN_BUILD_SCRIPT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/frontend-build.sh`:
- Around line 74-76: The rename of YARN_BUILD_SCRIPT to APP_YARN_BUILD_SCRIPT is
breaking for external callers; add a deprecation shim to preserve backward
compatibility by checking if YARN_BUILD_SCRIPT is set and, if so, set
APP_YARN_BUILD_SCRIPT from it (and log or echo a deprecation warning), while
keeping the new default export for APP_YARN_BUILD_SCRIPT; also add a brief note
to release notes/CHANGELOG calling out the rename and the temporary shim
(reference symbols: APP_YARN_BUILD_SCRIPT and YARN_BUILD_SCRIPT, and the docker
run -e forwarding change).
In `@universal_build.sh`:
- Around line 69-75: The change renames YARN_BUILD_SCRIPT to
APP_YARN_BUILD_SCRIPT which breaks consumers still setting the old var; update
the shell logic so that when APP_YARN_BUILD_SCRIPT is unset but
YARN_BUILD_SCRIPT is set, the script uses YARN_BUILD_SCRIPT and emits a clear
deprecation warning to stderr (e.g., echo >&2) advising to switch to
APP_YARN_BUILD_SCRIPT, otherwise default to running yarn build:prod; target the
if-block that checks APP_YARN_BUILD_SCRIPT and add the fallback/deprecation
handling for YARN_BUILD_SCRIPT.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fd561b11-9918-40d4-9ec0-a032df64f452
📒 Files selected for processing (4)
Dockerfilesrc/frontend-build.shtest/README.mduniversal_build.sh
| if [ -z "$NPM_BUILD_SCRIPT" ]; then | ||
| export NPM_BUILD_SCRIPT="build" | ||
| fi | ||
| if [ -z "$YARN_BUILD_SCRIPT" ]; then |
There was a problem hiding this comment.
@jgyselov thank you for the enhancement!
Removing these env vars will cause current consumers of the common builder utilities to start failing (at least those who use yarn).
Would it be possible to keep the existing env var, add a new one (such as YARN_NEXT_BUILD_SCRIPT), and then short circuit/abort setting the legacy env var if the new one is set?
2 participants
We would like to use the frontend builder for our project which uses Yarn 3.
The problem is that the
YARN_BUILD_SCRIPTvariable is treated as a Yarn config variable and it results in different behavior with Yarn 1 and Yarn 2+.Renaming the variable fixes this issue.
The other problem is the network timeout workaround which also doesn't work with Yarn 2+:
The easiest solution would be removing it completely (but I'm open to looking at other options).