Support for locally installed binaries

[lishaduck]

I just realized that my patch in 43942a121b77a3c12aa96f44104773f2216a6664 masks the real issue, that elm-review assumes that if you've used npm to install Elm, you're also running elm-review via npx. This assumption isn't true, and we should modify the actual code to get elm from ./node_modules/bin, and also ../node_modules/bin, etc.

Nonetheless, I'd argue that that patch is at least a band-aid, and this can be deferred for a bit as no-one has reported this in the wild as far as I know.

EDIT (for @lydell): For a "real" use case, the tests install elm-review globally, but elm locally.

[lydell]

Note that when calling elm-format we do this: https://github.com/jfmengels/node-elm-review/pull/49

So if this is wanted for the elm binary too, you could copy that.

Just out of curiosity, how are you calling elm-review? In a way where the current approach doesn’t work. node-test-runner and elm-watch have the same assumptions.

[lishaduck]

Note that when calling elm-format we do this: #49

So if this is wanted for the elm binary too, you could copy that.

Just out of curiosity, how are you calling elm-review? In a way where the current approach doesn’t work. node-test-runner and elm-watch have the same assumptions.

I don't have elm globally installed. In in-editor tests and when running the test script directly, it fails. I know some people prefer to skip npx and just directly call ./node_modules/.bin/..., so there's an actual use-case (though I doubt that the speedup helps given the extra typing). Oh, and global installation of elm-review probably doesn't work without elm installed globally? (is it an npm dep no)?

Anyway, not important, it's not vulnerable or anything, just that it might be nice to account for weird corporate edge cases/beginners to node installing things funnily (me a few years ago 🙃)

[lydell]

The thinking has been that if somebody installs elm-review globally, they’re gonna install elm globally too.

The ./node_modules/.bin/elm-review thing is more likely, though. I used to put ./node_modules/.bin in $PATH to have all local tools available without npx or anything. (I stopped using it, though, since it doesn’t work if you cd into a subdirectory of the repo.) These days I use a small bash replacement for npx: https://github.com/lydell/dotfiles/blob/29877e7903e997bebab97e6d60551e53d5b15bf2/bin/x.

It’s a pretty nice simplicity to the convention for tools to just require that elm is in $PATH, though. It’s up to the user to make sure it is. Either by having a global install of Elm, or by using npx, npm run or node --run or similar. While it’s pretty easy to add all the node_modules/.bin folders to $PATH, it feels slightly weird to implement that in every tool without a strong use case. I haven’t heard anyone complain about it. But on the other hand, maybe the few people who would complain about it worked around it instead.

[lishaduck]

The thinking has been that if somebody installs elm-review globally, they’re gonna install elm globally too.

Yeah. Again, I'd agree, but then someone's going to file an issue someday, so we might as well smoothen the path. And if we already have the code for elm-format, it should be trivial to fix.

The ./node_modules/.bin/elm-review thing is more likely, though. I used to put ./node_modules/.bin in $PATH to have all local tools available without npx or anything. (I stopped using it, though, since it doesn’t work if you cd into a subdirectory of the repo.) These days I use a small bash replacement for npx: https://github.com/lydell/dotfiles/blob/29877e7903e997bebab97e6d60551e53d5b15bf2/bin/x.

That's cool, thanks for sharing. I might have to add that to my dotfiles.

It’s a pretty nice simplicity to the convention for tools to just require that elm is in $PATH, though. It’s up to the user to make sure it is. Either by having a global install of Elm, or by using npx, npm run or node --run or similar. While it’s pretty easy to add all the node_modules/.bin folders to $PATH, it feels slightly weird to implement that in every tool without a strong use case. I haven’t heard anyone complain about it. But on the other hand, maybe the few people who would complain about it worked around it instead.

I think I ran into this a long, long time ago, but worked around it. (where the "me a few years ago 🙃" comment came from)

[lydell]

but then someone's going to file an issue someday, so we might as well smoothen the path.

It’s been years and nobody has asked for it. Sounds like feature creep?

And if we already have the code for elm-format, it should be trivial to fix.

The reason it exists for elm-format is purely accidental. But, yeah, trivial to “fix” since we already have the code in one out of two places.

[lishaduck]

but then someone's going to file an issue someday, so we might as well smoothen the path.

It’s been years and nobody has asked for it. Sounds like feature creep?

Fair enough, I'd close this issue... but as you said below ↓

And if we already have the code for elm-format, it should be trivial to fix.

The reason it exists for elm-format is purely accidental. But, yeah, trivial to “fix” since we already have the code in one out of two places.

I'd also think it's a bit unpredictable that they're different. I think you've otherwise convinced me this isn't worth it, except for that. If elm-format does it then so should elm. We maybe should drop that with old nodes (as it's a major).

Oh, and I still think that the original reason I noticed this, the tests, should be fixed. Again, with predictability, I'd prefer to add it to the actual code and not just the tests. If the code is there, the cost is low, and in return it's predictable. I'd be fine with dropping special casing elm-format though, I guess. I just don't want to have to globally install elm and elm-format to be able to run ./test/run.sh

[lydell]

Does that mean that you run ./test/run.sh directly instead of via npm run test-run?

[lishaduck]

Does that mean that you run ./test/run.sh directly instead of via npm run test-run?

Sometimes. I mostly use turbo, but when I was writing #193, I did. Then I got it in my head, so I've run that (quite) a few times and when it fails, it trips me up. Would be nice to just support that "out of the box" (for testing, which isn't in a box, I guess).