fix: load dev-only dependencies during analysis - #857
Conversation
doorgan
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I'm not sure this is a safe direction though. My main concern is that the two calls to Mix.Task.run(:loadconfig) merges the dev and test configurations, which I can see causing problems. There's likely other subtle issues stemming from this or by compiling code in both envs(different envs definining modules with the same name is one that comes to mind)
|
That's a fair concern. Running loadconfig for both envs does merge the dev and test configs, and I don't have a clean way to keep them separate without changing how the analysis workspace picks its env, which is a bigger change than this PR should carry. Happy to close this one. If there's an approach you'd accept, for example preparing deps for a single configured env, or only resolving dev-only path deps without loading their config, I can rework it against that. |
|
@mvanhorn I don't know yet how we can tackle this, it's a hard problem. ElixirLS for example just lets you configure which env to use, which is not perfect but it's a useful escape hatch, we can implement that in the meantime. |
|
That matches my read of it, and I would rather not push this shape further until you have decided the direction. What this PR does is take the union of dev and test dependencies in the isolated build workspace. That is implicit — it picks for the user and it widens what gets compiled for everyone, including people who never hit the problem. The configurable escape hatch you describe is the opposite trade: explicit, opt-in, and it cannot regress anyone who leaves it alone. For something we do not yet understand the root of, the second is the safer thing to ship, even though it pushes the decision onto the user. On prior art, the pattern in other ecosystems does seem to be "let the user say what to build" rather than inferring it: gopls exposes build flags and environment for exactly the build-tag and env-specific-code cases, and rust-analyzer exposes cargo feature selection including an all-features setting. Both landed on configuration rather than trying to guess the union. That is a shallow read though — I have not gone through their issue history to see what they rejected on the way there, which is where the useful part probably is. Happy to close this and open a focused PR for the configurable env instead, or to leave it open while you think. Just say which — I do not want to keep a conflicted PR sitting in your queue if the approach is not the one you want. |
Extend the existing initial dependency-preparation path in
Engine.Build.Projectso Expert prepares the union of dependencies active in the development and test environments inside Expert's isolated build workspace. Expert runs project analysis withMix.env(:test)so it can compile test helpers and test-only code.A temporary Mix project references a local path dependency with
only: :dev; after Expert's initial build, project code and an.exsdocument can resolve that module without a missing-module diagnostic; The same project references a different local path dependency withonly: :test; the normal test-environment compile still resolves it and test-helper analysis remains available.Fixes #533