aadsm / jsmediatags

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

Add options for XhrFileReader #99

Closed akiraohgaki closed 3 years ago

akiraohgaki commented 5 years ago

Because I need access to files via webdav. and added two options:

Example:

jsmediatags.Config.setXhrHeaders({
  'Authorization': 'Basic dXNlcm5hbWU6cGFzc3dvcmQ=',
  'X-Foo': 'bar'
});
jsmediatags.Config.setXhrWithCredentials(true);
aadsm commented 5 years ago

Thanks for create this PR! This is a welcome addition but I’m concerned how this applies different domains. This assumes the same set of http headers will make sense for all domains when that’s probably not the case. My suggestion is to add a domain string to the 2 functions: setXhrHeaders(“example.com”, {...}), what do you think?

I’ve also updated the test suite (they were failing), if you can rebase into master and add a test for this new feature, that would be awesome!

akiraohgaki commented 5 years ago

I think if more options support in "Reader" class constructor, that would be nice, programalable, and better than my PR.

Maybe like this:

const uri = 'https://example.com/path/to/file';
const options = {};

if (uri.startsWith('https://example.com')) {
    // for XhrFileReader
    options.xhr = {
        headers: {
            'Authorization': 'Basic dXNlcm5hbWU6cGFzc3dvcmQ=',
            'X-Foo': 'bar'
        },
        withCredentials: true
    };
}

new jsmediatags
    .Reader(uri, options)
    .read({
        onSuccess: (tag) => {
            console.log(tag);
        },
        onError: (error) => {
            console.error(error);
        }
    });
aadsm commented 5 years ago

Sorry to come back to you so late. Yeah, that makes a lot of sense, I’d be happy with that.