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

allow for plugins not using the ElixirSense.Plugin @behaviour #167

Merged
merged 3 commits into from
Oct 29, 2022

Conversation

zachdaniel
Copy link
Contributor

@zachdaniel zachdaniel commented Oct 21, 2022

So the problem with the existing implementation of plugins that live outside of ElixirSense (a problem that I created 😄 ) is that, unless you want your external hex package to have an explicit dependency on elixir_sense, you're going to have to do something like this:

{:elixir_sense, ..., only: [:dev, :test]}

And then in your code that defines the plugin, you'll need to do something like:

if Code.ensure_compiled?(ElixirSense.Plugin) do
  defmodule MyApp.Plugin do
  
  end
end

If you don't do that, then users who use your plugin without including {:elixir_sense, ...} as a dev dependency will get errors.

BUT then you run into the issue that if they don't include {:elixir_sense, ...} as a dev dependency, then the plugin won't be compiled at all, and so elixir_sense won't know about it.

This became an especially big problem when elixir_sense updated and a bunch of new ash users that were pointing at elixir_sense on github got that new update, but that update was not compatible with the version of elixir_ls that they were running. And so elixir_ls broke for all of them (and will break for any new user who follows the current getting started guide).

It is an unfortunate set of circumstances, and as far as I can tell there is really only two ways it can work and still be a good experience for end users:

  1. plugins can live in elixir_sense. This is great for really popular things like the ecto plugin, but personally I think it stifles creativity/velocity for other people who want to experiment with these things. Maybe thats only me, for now, but eventually I think it won't be

  2. plugin authors can do some hacky/annoying stuff to not have a dependency on ElixirSense, but to also not conditionally compile on its presence. Things like changing ElixirSense.Plugin.foo(bar) to apply(ElixirSense.Plugin, :foo, [bar]). Additionally, to avoid warnings on missing @behaviour modules, we have to make them findable without adopting the behaviour.

For an example of what this looks like for Ash (well, technically for Spark), you can see the PR that would make this work:
https://github.com/ash-project/spark/pull/5/files

Its naturally not ideal for me to have to code the plugin this way, but this is very much preferrable to the alternative of requiring users to pin a specific version of elixir_sense, IMO.

@lukaszsamson
Copy link
Collaborator

I totally agree that including elixir_sense as a dep isn't what most projects should do. Two things to keep in mind:

  1. See Deprecating ElixirSense module and providers #166. I don't have a concrete plan yet but it would be best if the interface was flexible enough to make the transition simple
  2. I plan to cut a new elixir-ls release soon (probably in two weeks) and I guess you would like to see that included

@zachdaniel
Copy link
Contributor Author

I would love to have this in the next release, but I also understand that you have your own prioritization going on. I am pretty confident that there are no other libraries doing what I'm doing yet, so I highly doubt it will present any issues to make this change. Especially because all of their code keeps working assuming their users include elixir_sense as a dependency.

@zachdaniel
Copy link
Contributor Author

This change would also make elixir sense pretty much irrelevant in plugins, excepting any elixir sense code they call. So just one upgrade step of "call into elixir ls instead", but otherwise nothing structural.

@zachdaniel zachdaniel marked this pull request as ready for review October 25, 2022 18:09
@zachdaniel
Copy link
Contributor Author

zachdaniel commented Oct 28, 2022

Hey @lukaszsamson so I think this is honestly good to go. I can merge the spark plugin changes, and my users and users of the ecto plugin shouldn't see any issues before/during/after upgrade.

@lukaszsamson lukaszsamson merged commit 8acc87c into elixir-lsp:master Oct 29, 2022
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

Successfully merging this pull request may close these issues.

2 participants