feat(vim.net): vim.net.download

[TheLeoP]

As stated in #29104 (comment) this PR aims to simply provide vim.net.download.

Notes:

• What minimal version of curl should be supported? I annotated the options that were introduced in a specific version (at least the ones that man curl mentions) in the code for now (e.g. (Added in 7.88.0))
• Only downloads via GET are being handled right now. I wanted to ask for feedback before adding support for other methods.
• What auth/proxy options should be supported? Right now, the PR only contemplates the basic options for both
• Are the current defaults ok? I copied the curl defaults (i.e. curl does not follow redirects by default, so neither does vim.net.download).
• What curl options shouldn't be supported? I introduced options like compressed that may be overkill (?). For example, it seems like the curl shipped with Windows did not supported this option (at least when it was first introduced. The shipped curl in my windows machine does not complain when I use --compressed)

[TheLeoP]

Huh, it looks like Ubuntu 22 has curl version 7.81

[clason]

Maybe as or to or target would be shorter? People will specify this virtually always.

[clason]

Does this also support specifying a target directory? I think this would be useful, too.

[TheLeoP]

Does this also support specifying a target directory? I think this would be useful, too.

Currently, no. A workaround may be to let users pass a cwd to execute curl there (and hence download the file on that directory)

[clason]

Regarding curl, I think we have to take what we get: check version (and features! distros can and do compile the same version differently) and fail gracefully if an requested option is not supported. In my opinion, this is the most important feature a vim.net.download would provide over vim.system(curl):wait()!

Regarding flags and options, I would start simple and add things on request. From painful experience with nvim-treesitter, proxies are a very common request, so that should work out-of-the-box. (We also need to follow redirects because of Github moving cheese frequently (repo renaming etc.))

One thing we could add as part of this PR is use it for downloading spellfiles (mostly as a way for free testing).

[clason]

I think we need to also discuss global configuration, as things like proxy settings are user-specific, while the caller of vim.net.download will in general be Nvim or a plugin.

Something like vim.net.config that takes a table of default options to download() (or later request) which the actual passed table extends? If in doubt, we should model this after vim.diagnostic.config.

[TheLeoP]

I was adding checks for curls versions and I had a thought (regarding the returned metadata mostly). I fell like this may be the wrong approach (?). If a plugin (or even neovim core itself) relies in feature A that's not available in some user's curl installation, vim.net.download will fail gracefully with a message stating that feature A is not available. Should we encourage this? Or shoud this features be excluded from vim.net.download?

The main examples I can think of right now are:

• not overriding a previous file with the same name as the downloaded one being available since curl 7.83.0
• json formatted metadata being available since 7.70.0
• json formatted request headers metadata being available since 7.83.0 (currently removed because Ubuntu 22 has version 7.81)

For some features, the download could continue with a warning (metadata), but for others (avoid overriding files) it may be dangerous to do so.

[dundargoc]

Let's not overcomplicate this. Pick a minimum version that includes all features we may want, and then abort with error if that minimum version isn't available.

[TheLeoP]

Is there some requirement for picking a minimal curl version? What is the oldest know curl in a supported platform?

[TheLeoP]

Another question. My initial implementation of spellfile.lua is async, but it looks like when :set spell triggers the SpellFileMissing event, Neovim expects the spell file to be downloaded before the autocmd callback has finished (i.e. sync). Would it be ok to use vim.wait for this? Or should something else be changed?

edit: #3027 seems to simply :set spell again at the end, so that may be a solution (?)

[dundargoc]

Is there some requirement for picking a minimal curl version?

Whatever version is needed to allow vim.net.download to do what it needs to do. If you can't come up with a minimum version for now don't force it, it should become apparent anyway when some operation doesn't work.

What is the oldest know curl in a supported platform?

Doesn't matter.

[justinmk]

• This again looks like it's getting overcomplicated. And no, it's not because "the topic requires it to be complicated".
  • netrw downloads spellfiles and :edit https://... files and I've never had to set any configuration. That is the initial goal here.
  • The first step is just to download files for the common case. Do not worry about proxies or other edge cases. Those can be considered the next phase. Not this phase. (And we may decide to tell users to configure a curl rc file, instead of duplicating all of that in Nvim.)
• Why do we need both download() and fetch()/request()? can we have 1 function with multiple "overloads"?

[TheLeoP]

Why do we need both download() and fetch()/request()? can we have 1 function with multiple "overloads"?

We don't need both, they do behave very similarly. The last commit on #29104 only uses fetch .

[TheLeoP]

@justinmk is there something else you would like to be removed from the interface?

[justinmk]

A typical pattern is that passing a callback makes it async, and omitting a callback makes it synchronous.

Something to ask is, why is the as (aka file) option necessary? Is it awkward for the caller to write the result to a file using Nvim's stdlib? If so, why, and can we fix that? I would expect it to be as simple as:

-- Asyndc
vim.net.request('https://...', {}, function(r)
  vim.fn.writefile(r.data, '/my/file')
end)

-- Sync
local r = vim.net.request('https://...', {})

Also: we are currently figuring out a long-term story for promises/async. But until then, we should try to use common patterns. vim.system() returns an object that provides :wait(). Should we mimic that pattern here? I think I prefer the "optional callback" pattern of libuv/luv, but @lewis6991 may be able to comment on whether the :wait() pattern has advantages.

[TheLeoP]

Something to ask is, why is the as (aka file) option necessary?

I haven't thought this thoroughly. The first reason I can think of is that it may not always be desirable to need to pass the downloaded file through Neovim to write it. Downloading a big file may cause memory issues (?) (but then, this may be an edge case(?))

[lewis6991]

Advantages of wait() is that it:

• makes it slightly easier to type annotate as the object always returns the same type.

• generally more versatile. I.e. you might want to submit multiple tasks and then wait on all of them. It's possible to do this with the other method of providing a dummy callback, but is ugly.

   local a = foo(function() end)
   local b = foo(function() end)
   local c = foo(function() end)
  
   a:wait()
   b:wait()
   c:cancel()

I think there is merit in the other pattern, and I don't think we need to restrict ourselves to either one.

I think for vim.net, the wait() pattern might pan out better longer term. For low level uv functions, the other pattern works well.