Skip to content

fix: dont override $.prefix setting defined via envs, revert `env.S… #1261

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.

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

Merged
merged 4 commits into from
Jul 9, 2025

Conversation

antongolub
Copy link
Collaborator

@antongolub antongolub commented Jul 9, 2025

closes #1260

Fixes setting $.prefix as ZX_PREFIX="" env.

  • Tests pass
  • Appropriate changes to README are included in PR

@antongolub antongolub changed the title fix: dont override $.prefix setting defined via envs, Revert `env.S… fix: dont override $.prefix setting defined via envs, revert `env.S… Jul 9, 2025
@antongolub antongolub requested review from antonmedv and Copilot July 9, 2025 07:15
Copilot

This comment was marked as outdated.

@antongolub antongolub requested a review from Copilot July 9, 2025 10:29
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR makes prefix and postfix optional in the core API, updates default handling, and ensures existing $.prefix/$.postfix values from the environment are not overridden. Key changes include:

  • Marking prefix and postfix as optional and removing their empty-string defaults
  • Updating fullCmd to safely handle undefined prefix/postfix
  • Extending the initial $ destructuring to include prefix and postfix

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/core.ts Made prefix/postfix optional, updated defaults, fullCmd, and destructuring logic
build/core.cjs Mirrored src/core.ts changes in compiled output
.size-limit.json Bumped size limits to reflect the new build

src/core.ts Outdated
useBash()
if (isString(shell)) $.shell = shell
if (isString(prefix)) $.prefix = prefix
if (isString(postfix)) $.postfix = prefix
Copy link
Preview

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

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

This line assigns prefix to $.postfix instead of postfix. It should be $.postfix = postfix.

Suggested change
if (isString(postfix)) $.postfix = prefix
if (isString(postfix)) $.postfix = postfix

Copilot uses AI. Check for mistakes.

build/core.cjs Outdated
useBash();
if ((0, import_util.isString)(shell)) $.shell = shell;
if ((0, import_util.isString)(prefix)) $.prefix = prefix;
if ((0, import_util.isString)(postfix)) $.postfix = prefix;
Copy link
Preview

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

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

This assignment uses prefix instead of postfix. Update to $.postfix = postfix.

Suggested change
if ((0, import_util.isString)(postfix)) $.postfix = prefix;
if ((0, import_util.isString)(postfix)) $.postfix = postfix;

Copilot uses AI. Check for mistakes.

@antongolub antongolub merged commit 523ef8e into google:main Jul 9, 2025
28 checks passed
@antongolub antongolub deleted the fix-shell-init branch July 9, 2025 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using the SHELL env variable is a breaking change
2 participants