Feature: Don't check version if path is present

[Gigitsu]

I think checking the existence of the version param is redundant when using a path for esbuild binary. In this case, esbuild is probably managed with an external package manager (eg. npm).

[josevalim]

I think we should check the version still, because you want to guarantee all coworkers use the correct one. We would accept a way of setting the version to nil or false, but it may already be supported.

[Gigitsu]

It doesn't support setting the version to nil or false.

However, I think I wrote the description wrong because if you set the version, it is still checked. This PR enables you to omit or set the version to nil or false if there is a path, without complaining. But it will still check the version if it is set.

[josevalim]

I don’t think or will work here.

[Gigitsu]

You are right, fixed

[josevalim]

I am worried this will be a breaking change and I also believe the code does not work as you expect. I do t think we should change configured_version() to return anything else other than the configured version.

[Gigitsu]

I've fixed the code, sorry about that.

I am worried this will be a breaking change

I thought about it, and I can't see any breaking changes except the lack of a warning log (which is what I want to achieve with this PR). Without these changes, if you don't set a version, you always get a warning. Now you get a warning only if you don't set a path as well.

The other difference is the default version printed in the warning log. But, in my opinion, if you set the path and a version, it's correct that the default version is the bin one.

There is no changes in the behaviour of the library as far as I can tell, unless I'm missing something :D

[josevalim]

But, in my opinion, if you set the path and a version, it's correct that the default version is the bin one.

That's breaking change. Now someone can update the version and a co-worker will still use the old version. As I said, we can have an option to explicitly disable the check, but I don't think we should change the return value of configured_version.

[Gigitsu]

Now someone can update the version and a co-worker will still use the old version.

There's probably something I'm missing, but when can this happen?

There are 3 scenarios, as far as I can tell:

1. You don't set either :version or :path -> it behaves as before
2. You set only :path -> no warnings are printed (this is what this PR aims to achieve)
3. You set both :version and :path -> if the version you set differs from the version of the installed bin, you get a warning like before.

I thought you were concerned about point 3. but in that case, it behaves as before.

The value returned by configured_version() changes only in scenario 2. when you set :path and don't set :version.

[Gigitsu]

But I can see why you wouldn't want to change the behaviour of configured_version.

With my code it doesn't return the "configured" version anymore but something more like a "target" or "desired" version.

What do you think about changing the name of that function?

[Gigitsu]

Hi @josevalim, can I do something to improve this PR?

[josevalim]

This comment is still relevant: #73 (comment)

[Gigitsu]

Ok so I'll try to add an option to explicitly disable the check. Could version_check: true | false work for you?

[Gigitsu]

PR updated. I want to add a test but I don't know how to do it. I thought to download an older version, and then run the esbuild process capturing the output, but I'm not sure this is the right approach.

[josevalim]

Please update the docs here and we should be good to go: https://github.com/phoenixframework/esbuild/blob/main/lib/esbuild.ex#L22

[Gigitsu]

Updated

[josevalim]

💚 💙 💜 💛 ❤️