Run elm-review in a custom watch mode

[jmbockhorst]

@jfmengels I've started work on trying to run elm-review using a custom runner inside the language server that can make use of watch mode. It significantly improves performance as expected, and it can show diagnostics even without file save. It is a simple implementation right now and is missing features of the normal elm-review watch mode (like files getting added, elm.json changing, etc). What do you think of this approach?

[socket-security[bot]]

New dependency changes detected. Learn more about Socket for GitHub ↗︎

---

👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary

Issue	Status
Install scripts	✅ 0 issues
Native code	✅ 0 issues
Bin script shell injection	✅ 0 issues
Unresolved require	✅ 0 issues
Invalid package.json	✅ 0 issues
HTTP dependency	✅ 0 issues
Git dependency	✅ 0 issues
Potential typo squat	✅ 0 issues
Known Malware	✅ 0 issues
Telemetry	✅ 0 issues
Protestware/Troll package	✅ 0 issues

---

📊 Modified Dependency Overview:

➕ Added Package	Capability Access	+/- Transitive Count	Publisher
[email protected]	network, filesystem, shell, environment	+57	jfmengels

[jfmengels]

Hey @jmbockhorst 👋

Nice work! It warms my heart to see work being done on the integration for elm-review ❤️

it can show diagnostics even without file save This is awesome 😍 I did not think this would be possible in practice.

What do you think of this approach?

While the results seem great, I think it's an interesting approach but one that will have flaws, though we may work around some of them.

1. The first issue is that I (and the CLI's codebase) am not prepared to have the internals of the CLI be backwards compatible. When adding features in the future, I will likely change the implementation in ways that will break VSCode. Having to maintain both will be a maintenance burden for both me and you, the maintainers of the language server.

2. You're adding a dependency for node-elm-review, and limiting to a specific version. This means that every time I release a new version, you should likely release a new version of LS as well, even if only to update the version in your package.json. This also means that elm-review users might use two versions of elm-review in practice: v2.9.2 defined in the LS (or whatever version you will bump it to in the future), and the one that they have in their own package.json. Having two different ones might lead to different results which I'm sure can be a source of frustration.

3. I'm currently looking a lot at caching. I'm for instance trying to figure out if it's possible to run elm-review where I only load some files in memory (and more on demand when necessary). I don't think it's going to be a blocking problem but I'm wondering how that would work with such an integration.

4. I'm not sure that elm-review will always remain a JS CLI. I don't plan on rewriting it now, but it's something I keep my eye out for (using a forked version of the compiler to get things like type information, or using Rust for better performance). Not sure how this will work if/when that happens. I wonder how it works for other linters 🤔

Solution proposals

All that said, I think there are things that can be done that would be a nice mix between the two.

For 1), while it may take some time and design, I think we could make some kind of public JS API that you depend on. That would simplify the integration in here, while making boundaries for me where I can determine what I can or cannot change. And if I ever need to change the public JS API in a breaking way because a new feature requires it, then I can make a v2 of the public API and deprecate v1 while keeping it somewhat working. Or just have v1 report a global-error/exception saying that the user needs to update their version. Not entirely sure yet, but these are possibilities.

I think that with the approach you're using currently, you can explore what the differences with the CLI are (for instance, you need to inject contents of files that are not on the file system), and then we can figure out what a nice API that supports everything would be. With the current PR, it's a bit hard for me to tell at a glance what has or hasn't changed.

For point 2) about adding node-elm-review as a dependency, I think it's possible that you try to — on-the-fly — import node-elm-review's internals like you do, based on the path of elm-review. So instead of calling the CLI like you do in the current released version of Elm with a path that was configured, you import it based on the same path (maybe the path to the node_modules needs to be tweaked a bit).

---

I think it could also be fine to continue with the current approach, even merge it and release, but it should probably not stay like this in the long run. You might also want to report errors if the user's version of elm-review is (too?) different from the one LS is using.

What do you think? Does this sound like an interesting approach? Am I be missing something obvious? And once again, thank you for looking into this ❤️

[jmbockhorst]

Thanks for the detailed response!

On points 1 and 2, I think a public API would be great and we can work towards that. As you mentioned, the language server will still have to support multiple versions to maintain backwards compatibility, whether it is a public v1/v2 api or not for now. If we are able to use the user's version of node-elm-review instead of the language server's version, then we would only need to update the language server if the APIs we are using change, and support multiple version in the language server.

On point 3, ideally we would actually want to show errors for the just the current/open files first, then the rest. Since we still have the long startup time before seeing the first error on first run.

where I only load some files in memory (and more on demand when necessary)

I wonder if this is a similar idea.

On point 4, theoretically we could use web assembly bindings to a public api of the rust or haskell runner and still run from the language server.

Again, thanks for taking the time to think through this!