aadsm / jsmediatags

Media Tags Reader (ID3, MP4, FLAC)
Other
748 stars 128 forks source link

Add the ability to load a relative URL #83

Open captbaritone opened 6 years ago

captbaritone commented 6 years ago

I would like to be able to load a file via a relative URL such as /built/mp3/llama.mp3. Currently this does not work, because the XhrFileReader.canReadFile() thinks it can only read URLs that:

  1. Start with a letter.
  2. Contain a :.

I can think of two possible ways to achieve this:

1

Change the RegEx in XhrFileReader.canReadFile() to have a more permissive definition of a URL, which could include relative URLs.

The current RegEx is a bit naive, so this might be a good improvement, but on the other hand, depending upon our definition, a relative URL could theoretically be as simple as file.mp3 or even file_without_an_extension, we may not be able to be much more selective than "is it a string?".

The down-side of this is that it could represent a breaking change for someone else who had a custom MediaReader that used a string as its file.

2

Expose the three browser readers as properties on the jsmediatags object. This would allow me to do something like:

const reader = new jsmediatags.Reader();
reader.setFileReader(jsmediatags.XhrReader);
reader.read("/built/mp3/llama.mp3", ...);

Being able to explicitly use one of the built-in media readers seems like a useful option.

3

Some other, possibly obvious, solution that I haven't seen so far.


I'd be happy to open a PR for either any of the above. Thanks for your work on this! I'm excited to finally get proper ID3 (etc) support in Winamp2-js!

aadsm commented 6 years ago

Just to be precise, the XhrFileReader reads URLs for strings that start with <one or more letters>://. At the time I made the decision to only use protocol:// for this reader because this is something the consumer of the library always knows about.

In your case if you know that your paths starting with / will always be local to the hostname you can do jsmediatags.read(window.location.origin + filepath, ...). Another alternative is to create a wrapping function:

function readTags(filepath, ...args) {
  if (filepath[0] === ‘/‘) {
    filepath = window.location.origin + filepath;
  }
  return jsmediatags.read(filepath, ...args);
}
readTags(‘/build/mp3/llama.mp3’);
captbaritone commented 6 years ago

That’s basically the work around I have in place. https://github.com/captbaritone/winamp2-js/blob/master/js/actionCreators.js#L260

This work around is probably fine for me for now. In the future, I might want to let users supply a URL, and I’d rather not have to handle all the possible types of URLs that lack protocols: relative URLs, absolute URLs, protocol relative URLs (I think that’s all?)

Since I know that the input is a url in this case, it would be a bit better to be able to tell that to jsmediatags directly, rather than having to change the input so that it can infer the type.

Again, it’s possible to work around this, so I’m not blocked, but it does add a bit of cruft on my end.

Thanks again for the great library.

Jordan Eldredge (mobile)

On Jan 6, 2018, at 8:49 AM, António Afonso notifications@github.com wrote:

Just to be precise, the XhrFileReader reads URLs for strings that start with ://. At the time I made the decision to only use protocol:// for this reader because this is something the consumer of the library always knows about.

In your case if you know that your paths starting with / will always be local to the hostname you can do jsmediatags.read(window.location.origin + filepath, ...). Another alternative is to create a wrapping function:

function readTags(filepath, ...args) { if (filepath[0] === ‘/‘) { filepath = window.location.origin + filepath; } return jsmediatags.read(filepath, ...args); } readTags(‘/build/mp3/llama.mp3’); — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

aadsm commented 6 years ago

Ah ok, I should have checked your PR before commenting :D. I’m fine with people having to add the function for this particular case, but it might be useful to add this to the documentation as a note so I’ll do it. Something I didn’t mention on my original comment is that this library doesn’t always run on the browser. So on node or other environments it’s not possible to make that request, I want to avoid this file reader reporting that it can in that situation.