Skip to content
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

Fix dev for Ruby apps #4522

Closed
wants to merge 3 commits into from

Conversation

gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Sep 25, 2024

WHY are these changes introduced?

Fixes #4507

In #4401 we added a mechanism to check the safety of external binary runs from the CLI, but it caused bin/rails commands to fail, making the dev command to crash with Ruby apps.

WHAT is this pull request doing?

Avoid the which function to raise errors, used to find out where a binary is.

How to test your changes?

Create a Ruby app and run dev

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@@ -121,8 +121,8 @@ Running system process:
}

function checkCommandSafety(command: string) {
const commandDirectory = dirname(which.sync(command))
if (commandDirectory === cwd()) {
const commandPath = which.sync(command, {nothrow: true})
Copy link
Contributor Author

@gonzaloriestra gonzaloriestra Sep 25, 2024

Choose a reason for hiding this comment

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

The nothrow makes it return null instead of raising an error when the command is not found. More info: https://github.com/npm/node-which?tab=readme-ov-file#usage

Copy link
Contributor

github-actions bot commented Sep 25, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
73.14% (+0.06% 🔼)
8415/11505
🟡 Branches
69.65% (+0.06% 🔼)
4105/5894
🟡 Functions 71.87% 2184/3039
🟡 Lines
73.49% (+0.06% 🔼)
7963/10835

Test suite run success

1897 tests passing in 862 suites.

Report generated by 🧪jest coverage report action from a84792f

Copy link
Contributor

@MitchDickinson MitchDickinson left a comment

Choose a reason for hiding this comment

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

A few questions first!

packages/cli-kit/src/public/node/system.ts Show resolved Hide resolved
packages/cli-kit/src/public/node/system.ts Show resolved Hide resolved
packages/cli-kit/src/public/node/system.ts Show resolved Hide resolved
@gonzaloriestra
Copy link
Contributor Author

Closing in favor of #4527

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting this error: not found: bin/rails when I run shopify app dev
3 participants