jfmengels / elm-review

Analyzes Elm projects, to help find mistakes before your users find them.
https://package.elm-lang.org/packages/jfmengels/elm-review/latest/
Other
251 stars 13 forks source link

Use elm-json from environment ($PATH?) instead of downloading it. #157

Closed jumper149 closed 1 month ago

jumper149 commented 1 year ago

I would like to run elm-review in a nix expression, but elm-review will try to download elm-json, which is impure and therefore not allowed from within nix.

Could elm-review use the elm-json that is already available in the environment?


On another note. Are there other cases where elm-review wants download anything?

jfmengels commented 1 year ago

Hey @jumper149,

We have had a number of issues with using elm-json, and we'd like to replace it. There is an issue about that in the CLI's issues (https://github.com/jfmengels/node-elm-review/issues/81). Help with it would be very welcome! We could potentially do what you suggested, but I think that this would be the ideal solution.

On another note. Are there other cases where elm-review wants download anything?

Yes.

1) Elm compiles your review configuration using the Elm compiler, which means dependencies will be downloaded to your ELM_HOME. 2) Using elm-json (and probably using the solution in https://github.com/jfmengels/node-elm-review/issues/81 as well) will under the hood try to update your review/elm.json to add dependencies. That might make some HTTP requests. 3) elm-review will try to find the dependencies of the project to be analyzed in your ELM_HOME. If they're not there, it will fetch those files it's interested in on its own, downloading them to somewhere in elm-stuff.

That's all I can think of right now.

jumper149 commented 1 year ago

I am not sure, if exchanging elm-json for elm-solve-deps-wasm will really help me in my case. I guess this issue is becoming more of a feature request now.

  1. Elm compiles your review configuration using the Elm compiler, which means dependencies will be downloaded to your ELM_HOME.

That shouldn't be a problem. Invocations of the elm compiler work fine with the nix setup: https://github.com/NixOS/nixpkgs/blob/0854c54046671a3e981a0716a1a6597710575656/pkgs/development/compilers/elm/fetchElmDeps.nix#L10 The dependencies are injected beforehand.

  1. Using elm-json (and probably using the solution in Use elm-solve-deps-wasm instead of elm-json node-elm-review#81 as well) will under the hood try to update your review/elm.json to add dependencies. That might make some HTTP requests.

It would be nice to have a flag to turn off that feature (at least for my usecase). Overall I think it's good to isolate side-effects from the rest of a program, but I guess I don't have to preach about purity to Elm users.

  1. elm-review will try to find the dependencies of the project to be analyzed in your ELM_HOME. If they're not there, it will fetch those files it's interested in on its own, downloading them to somewhere in elm-stuff.

Just like (1.) this should be fine.

zwilias commented 1 year ago

We do a fairly horrible thing where we patch elm-review to have let elmJsonPromise = Promise.resolve("ELM_JSON_PATH"); and then substituteInPlace that ELM_JSON_PATH with ${pkgs.elmPackages.elm-json}/bin/elm-json.

It's not that much fun, but we probably should upstream that to nixpkgs.

jumper149 commented 1 year ago

@zwilias Well, it's a pragmatic fix to the issue at hand.

Are you running elm-review from within nix? Can you confirm, that aside from the elm-json download it is working fine inside nix? I am guessing that your code is not open source?

Upstreaming to nixpkgs is always nice, but if there was a fix for elm-review I would rather just pin elm-review to a new release/feature branch. (Just thinking, that the time would be better spent on elm-review)

I haven't looked into the source of elm-review really, but I'm guessing that a new CLI flag for the elm-json path should not be too hard to implement. There also is the --elm flag for the compiler path, so I think it does make sense to add this. I would call this new flag --elm-json, but unfortunately that is a little bit confusing with the --elmjson flag (for the elm.json file).

I am not sure if I can find the time to do it anytime soon, but I'll keep it mind.

@jfmengels Even if you decide to use elm-solve-deps-wasm, it would be desirable to allow the user to set the path to the executable instead of just downloading it at runtime.

Edit: Or you could make it a build-time dependency instead of a runtime dependency. That way you would also avoid the problem.

lydell commented 1 year ago

Now there’s one more potential solution: Replacing elm-tooling with the @lydell/elm-json package.

lishaduck commented 1 month ago

Firstly, this issue should be in node-elm-review (oh, it is jfmengels/node-elm-review#81). Secondly, we've used elm-solve-deps-wasm for a bit now (jfmengels/node-elm-review#144).

jfmengels commented 1 month ago

Yes, this was solved in https://github.com/jfmengels/node-elm-review/issues/81 which has been released in elm-review v2.11 onwards.