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

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

Closed
jumper149 opened this issue May 2, 2023 · 7 comments
Closed

Comments

@jumper149
Copy link

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
Copy link
Owner

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 (jfmengels/node-elm-review#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 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.
  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
Copy link
Author

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
Copy link

zwilias commented May 4, 2023

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
Copy link
Author

jumper149 commented May 4, 2023

@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
Copy link

lydell commented Jul 26, 2023

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

@lishaduck
Copy link
Contributor

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
Copy link
Owner

Yes, this was solved in jfmengels/node-elm-review#81 which has been released in elm-review v2.11 onwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants