fivecar / react-native-background-downloader-queue

A wifi-aware background downloader that maintains a queue of files
MIT License
8 stars 0 forks source link

URL.pathname not implemented #23

Closed gorbypark closed 1 year ago

gorbypark commented 1 year ago

Current Behavior

App.tsx:

const downloader = new DownloadQueue({
  onBegin: (url, bytes) => console.log('Download started', url, bytes),
  onDone: (url, localPath) => console.log('Download finished', url, localPath),
  onError: (url, error) => console.log('Download error', url, error),
  onProgress: (url, fraction, bytes, totalBytes) =>
    console.log('Download progress', url, fraction, bytes, totalBytes),
});

(async () => {
  await downloader.init();
  await downloader.addUrl('http://212.183.159.230/10MB.zip');
})();
Error: URL.pathname not implemented

I narrowed it down to the .addUrl() function. I have react-native-background-downloader, react-native-fs and async-storage all working independently, so I don't believe it's a dependency issue.

fivecar commented 1 year ago

Ah - this is a URL polyfill issue -- which is why it works in Node environments (e.g. jest), and with anyone who has the polyfill, but not in all cases. Let me get a solution out to you soon. Thanks for reporting!

fivecar commented 1 year ago

@gorbypark : Could you let me know if installing the react-native-url-polyfill fixes this for you? It should — it's how I've gotten it working in other RN apps.

Let me add this to the README as a step (i.e. specifically that there should be a polyfill if URL.pathname isn't available). I don't think we should include it as a required peerDepedency, in that I believe people can sometimes use other polyfills. Perhaps we enhance the API to pass a pathnameFromUrl as an optional parameter, which they can provide via any polyfill they'd like -- that feels more flexible than requiring that polyfilling actually happen (e.g. if you don't want the rest of your app polyfilled).

Update: released a fix in v3.0.0, which took a compromise position of defaulting to not preserving extensions, but allowing anyone to pass a urlToPath callback that then preserves extensions. See below.

gorbypark commented 1 year ago

react-native-url-polyfill

Ah yes, I should have recognized that error! The polyfill seems to work, although I'm not getting any progress indications in the callback after adding the url. I haven't had much time to play with it, though, might be something on my end.

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 3.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

fivecar commented 1 year ago

Thanks for letting me know it works, @gorbypark . As a compromise to break as few people as possible, I've merged #25 into v3.0.0, just released. This one takes an optional urlToPath callback, which, if provided, saves files while preserving the server-side extension. If not provided, files will be saved without extensions (which is the only thing that URL.pathname was used for).

This changes the package's behavior, in that it means that by default, it no longer preserves file extensions. But that seemed like a better choice than to make everyone polyfill their app. The new version, if you want to preserve file extensions, just requires that you pass urlToPath: url => new URL(url).pathname if you already have a polyfill.

I really appreciate you reporting this. We continue the 100% code coverage streak with this change, including 100% branch coverage.