-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
Run elm-review in a custom watch mode #942
base: main
Are you sure you want to change the base?
Conversation
New dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No new dependency issues detected in pull request Bot CommandsTo ignore an alert, reply with a comment starting with Pull request alert summary
📊 Modified Dependency Overview:
|
Hey @jmbockhorst 👋 Nice work! It warms my heart to see work being done on the integration for
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.
Solution proposalsAll 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. 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 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 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 ❤️ |
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 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.
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! |
@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?