Add support for pnpm (#117)

kinland commented 1 year ago

This solves the issue I reported in #117

I have another branch where I am testing an updated test action.

Some of the tests are failing with pnpm and yarn because they don't have the exact feature set of npm. I'm out of steam to dig deeper tonight, but if I get the test suite running fully with appropriate tests disabled, I will open a second PR (or, if you haven't had time to review this yet, I'll add to this PR).

kinland commented 1 year ago

Current status of the tests:

✅ NPM: passing

⚠️ PNPM: same results on everything but macos-latest for some reason, where this particular test case doesn't fail

  210 passing
  17 pending
  1 failing

  1) [fail] it should fail
       if tasks exited via a signal:
         with correct exit code:

      AssertionError [ERR_ASSERTION]: should have correct exit code
      + expected - actual

      -false
      +true

      at /home/runner/work/npm-run-all2/npm-run-all2/test/fail.js:112:7
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

❌ YARN w/ windows or ubuntu:

  216 passing
  1 pending
  23 failing (macos yarn has 1 additional failure)

A lot of the yarn failures are things like:

Error: ERROR: Task not found: "test-task:package-config"
- I suspect this is an issue with install of yarn
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value
- I am not 100% sure, but it seems like yarn just ignores invalid arguments rather than producing an error. So chances are, the config tests need to be disabled for both PNPM and Yarn, not just PNPM

bcomnes commented 1 year ago

Cool thanks for looking into this. I'll do my best to take a look by this weekend. Approving CI in the meantime. My main target is supporting npm since its what ships with node. Yarn and pnpm support is definitely best effort. If this works for the most part, but a few random tests fail in yarn/pnpm mode, I don't mind adding skips in those cases, so long as the npm side doesn't loose coverage. Sometimes its just not worth someones time to figure out edge cases like that unless they are important.

kinland commented 1 year ago

Cool thanks for looking into this. I'll do my best to take a look by this weekend. Approving CI in the meantime. My main target is supporting npm since its what ships with node. Yarn and pnpm support is definitely best effort. If this works for the most part, but a few random tests fail in yarn/pnpm mode, I don't mind adding skips in those cases, so long as the npm side doesn't loose coverage. Sometimes its just not worth someones time to figure out edge cases like that unless they are important.

I figured as much, but I wanted to stave off possible issues in new versions not being detected :)

My current approach is to confirm that a given functionality is not available in either pnpm or yarn when it fails. If the functionality is not present, then skip the tests on that package manager. Unfortunately, it's kinda tricky to confirm that; I couldn't find a good table/resource that showed which commands are different or what functionality was not present, (and the pnpm docs don't really say "we don't do this thing that npm does" for the most part... notable exception being running arbitrary 'pre' scripts, which there's a config override for) so I'm mostly having to try and reverse engineer the tests into terminal commands that I can run.

I noticed with yarn that if I did stuff like yarn run --random=other scriptname it didn't give an error, which is why I suspect that the config override tests need to be skipped on yarn. I didn't realize you could even do that in npm until I started trying to dissect what was being executed, and I was having a hard time finding npm docs describing this behavior. If you could confirm that, that would be appreciated. Otherwise, if I can't conclusively say "this doesn't exist in yarn, even with a different syntax", we could just skip the tests anyway, but generally I'd prefer not to do that

Here's the most recent run (same as in the OP) https://github.com/kinland/npm-run-all2/actions/runs/6350407589

bcomnes commented 1 year ago

Ok these changes look acceptable, however the one thing I wish to change is that we should stay within the async context and not add sync io inside of the promise. I know it adds some ugly code overhead, however mixing the two can lead to confusing bugs and errors, sometimes even the fault of the runtime.

and the pnpm docs don't really say "we don't do this thing that npm does" for the most part... notable exception being running arbitrary 'pre' scripts, which there's a config override for

Right, this all characteristic of the "drop in replacement" marketing myth used around alternative tooling. These differences tend to get buried on purpose.

I noticed with yarn...

Honestly, I don't care about yarn at all. It offers no advantages over npm at this point, has wasted countless hours due to its subtle undocumented differences and is essentially abandonware now. The best thing that project could do would be to deprecate the v1 that everyone still uses and tell everyone to use berry or whatever. I'm open to removing yarn support if someone PRd it.

I'll put together a patch to adjust that one thing.

bcomnes commented 1 year ago

@kinland I made a small modification in https://github.com/kinland/npm-run-all2/pull/1

bcomnes / npm-run-all2

Add support for pnpm (#117) #120