search

arcanis / clipanion

Type-safe CLI library / framework with no runtime dependencies
https://mael.dev/clipanion/
1.12k stars 66 forks source link

Make Type Check In `validateAndExecute()` Stricter #102

Closed noseworthy closed 3 years ago

noseworthy commented 3 years ago

We were seeing an issue in yarn v3.0.2 when using the workspaces plugin where it was possible for a command's schema prop to be not undefined, but also not an array. The issue is captured in the yarn repository here: https://github.com/yarnpkg/berry/issues/3108

In order to ensure that applyCascade() is only called with valid arguments, rather than checking that cascade is not undefined, ensure that it is an Array. Otherwise, when we try to iterate over it, we get a Type Error stating that followups.every() is not a function.

merceyz commented 3 years ago

Shouldn't it instead be an explicit error that the type of schema is invalid and that an array was expected? Just silently ignoring it seems dangerous and will probably cause other issues that are even harder to debug

noseworthy commented 3 years ago

Shouldn't it instead be an explicit error that the type of schema is invalid and that an array was expected? Just silently ignoring it seems dangerous and will probably cause other issues that are even harder to debug

Yeah, great point. I thought about that, but I'm not familiar with this project and didn't want to do too much damage, haha. All I knew was that the check for undefined wasn't sufficient for what I was seeing. As per our discussion on https://github.com/yarnpkg/berry/issues/3108 I was running old yarn plugins with yarn v3 when I saw this issue so it's probably okay to ignore this.

I can update this to throw an error though. I'll take a look through the repo to see how it's done elsewhere and get an update out to you.

noseworthy commented 3 years ago

I've updated it, but like I said, I'm not super familiar with this project and so I don't want to dirty up your codebase. This check really shouldn't be necessary as the TypeScript compiler should hopefully catch any badly constructed commands. Feel free to close if you'd like, but if you have comments or any other changes you'd like to see, just let me know!

Thanks again for your work on this!

merceyz commented 3 years ago

I have no power in this repository so I'll leave that decision to @arcanis 👍

arcanis commented 3 years ago

That feels reasonable, especially for JS codebases 👍