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

[FEATURE] run-script with workspaces should short-circuit on script error #575

Open
1 task done
johndiiorio opened this issue Sep 27, 2021 · 18 comments
Open
1 task done
Labels
Enhancement new feature or improvement Release 8.x

Comments

@johndiiorio
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

run-script with the --workspaces flag will run all workspace scripts, regardless if one script exits with exit code 1. Please see the "Steps to Reproduce" section.

Expected Behavior

I would expect that run-script would short-circuit and not run the other scripts if one fails. Alternatively, a new flag (e.g. --short-circuit) could be introduced to run-script in order to maintain backwards compatibility.

Steps To Reproduce

Consider the following scenario:

  • A root package.json file has "workspaces": ["a", "b", "c"]
  • a/package.json has the script "build": "exit 1"
  • Both b/package.json and c/package.json have the the script "build": "exit 0"

Run npm run build --workspaces and note that all scripts are run.

Environment

  • OS: Windows 10
  • Node: 16.10.0
  • npm: 7.24.0
@johndiiorio johndiiorio added Bug thing that needs fixing Needs Triage needs an initial review labels Sep 27, 2021
@wraithgar wraithgar added Enhancement new feature or improvement Release 8.x and removed Needs Triage needs an initial review Bug thing that needs fixing labels Mar 17, 2022
@wraithgar wraithgar changed the title [BUG] run-script with workspaces does not short-circuit on script error [FEATURE] run-script with workspaces should short-circuit on script error Mar 17, 2022
@wraithgar wraithgar added the Agenda will be discussed at the Open RFC call label Mar 17, 2022
@john-landgrave
Copy link

Big upvote on this one. This is kind of a dealbreaker when building a monorepo in CI? If we just execute a npm run --workspaces package (or whatever your build/bundle command is) a package can fail and the CI build will still succeed because the exit code comes out 0.

FWIW, this is also the request in this StackOverflow question.

@johndiiorio
Copy link
Author

@john-landgrave I agree, this bug makes it impractical to build a monorepo in a CI with just npm. One possible workaround is to create a script (and add it to your root package.json) that reads the package.json workspaces array, then individually invokes npm run build --workspace <workspace> and checks the exit code before proceeding. That shouldn't be necessary though, the behavior should be built-in.

@darcyclarke
Copy link
Contributor

@johndiiorio Thanks for taking the time to share your idea! New ideas are always appreciated and are better suited for our RFC repo instead since that's the right place in order to get more attention from the rest of the team and the community.

I've transferred the issue to that repo for future discussion

@hapkecom
Copy link

hapkecom commented Jul 6, 2023

Hi, is there any update on this? I thinks it's still needed for the reasons described above ...

ernscht added a commit to merkle-open/frontend-defaults that referenced this issue Aug 21, 2023
due to missing feature in npm workspaces (npm/rfcs#575)
ernscht added a commit to merkle-open/generator-nitro that referenced this issue Aug 23, 2023
due to missing feature in npm workspaces (npm/rfcs#575)
@DavidSouther
Copy link

How would you expect npm to order script execution across packages?

@ljharb
Copy link
Contributor

ljharb commented Oct 5, 2023

Topologically; in order of the dependency graph. Order would need to be deterministic for siblings but wouldn't otherwise matter.

@johnnyshankman
Copy link

johnnyshankman commented Feb 15, 2024

Order already can be done deterministically @DavidSouther as i'm using workspaces in a repo where one pkg relies on the other, and therefore must be built first.

"workspaces": [
    "packages/common",
    "packages/cli",
  ],

common will always build first, then cli.

Update: I see what he was asking about now. Like many nested.

@johnnyshankman
Copy link

johnnyshankman commented Feb 15, 2024

@darcyclarke any updates on this feature request? seems like a pretty easy one to add, just an exit check before moving on to the next workspace. is there any in-progress PRs to review etc?

@ljharb
Copy link
Contributor

ljharb commented Feb 15, 2024

@johnnyshankman darcy is no longer at npm, and npm hasn't done RFC calls in like a year, so there's not likely been any progress.

It would definitely not be easy to add; determining proper topological ordering is not a trivial task.

@johnnyshankman
Copy link

johnnyshankman commented Feb 15, 2024

@ljharb thanks for the update.

i'm not so sure what you mean about the topologically ordering. meaning which workspace goes first? why does that need to change?

oh also, is there a better place to request?

@ljharb
Copy link
Contributor

ljharb commented Feb 15, 2024

Yes, you might have 100 child workspaces, some of which have no workspace deps, but some of which are part of a complicated dependency chain. There may even be a circular dependency.

@johnnyshankman
Copy link

johnnyshankman commented Feb 15, 2024

@ljharb how does it resolve the graph currently? why would we change that behavior? this would simply be an additional flag, no? users with complex graphs can simply not use the flag. they probably wouldn't want it anyways.

@ljharb
Copy link
Contributor

ljharb commented Feb 15, 2024

What I mean is, this feature only works properly in conjunction with topological sorting, and that is nontrivial to add.

When topological sorting is used, this feature should just be on by default, no flag.

@johnnyshankman
Copy link

johnnyshankman commented Feb 15, 2024

@ljharb ah yes i see what you mean now. yes 100% in a perfect world we do topo sort and keep this on 24/7. agreed! also thank you for the insight. makes sense now your worries. not as easy as i would imagine!

in the meantime, is it so bad to add a --fail-fast flag for --workspaces? those who have too complex of a repo for it to work would not use it

my monorepo is one layer deep but 5 pkgs wide, so this would be hugely beneficial, as each subpkg relies on the last. building all of them in order every time to the end is silly when the first one breaks.

@johnnyshankman
Copy link

also curious if you know a modern workaround that avoids a feature request!

@darcyclarke
Copy link
Contributor

@darcyclarke any updates on this feature request? seems like a pretty easy one to add, just an exit check before moving on to the next workspace. is there any in-progress PRs to review etc?

I'm no longer working on npm unfortunately. That said, & fwiw, I do plan to ensure this is a capability out of the gate in my new project 🤷‍♂️

@johnnyshankman
Copy link

@darcyclarke you're an OSS legend mate! good to see ya. look forward to that.

@BastianTrifork
Copy link

Any updates on this? Spending days migrating away from Lerna, then finding this feature missing is a huge heartbreak 💔
In regards to topo sorting, i think support for circular dependencies is going to be a lot of work that will help relatively few projects. A naïve approach might get more people to adopt workspaces, since i suspect this is a dealbreaker feature for many.

jpinkney-aws added a commit to jpinkney-aws/aws-toolkit-vscode that referenced this issue Dec 11, 2024
Problem:
- npm workspaces do not have a fail fast mechanism. See npm/rfcs#575. Meaning, if amazon q fails to build or tests fail, ci can still show as passed

Solution:
- Introduce a wrapper script that implements this fail fast behaviour
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement new feature or improvement Release 8.x
Projects
None yet
Development

No branches or pull requests

9 participants