Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail to fallback to "download the entire file" using remote URL's #7

Closed
ghost opened this issue Feb 10, 2016 · 9 comments
Closed

Fail to fallback to "download the entire file" using remote URL's #7

ghost opened this issue Feb 10, 2016 · 9 comments
Assignees

Comments

@ghost
Copy link

ghost commented Feb 10, 2016

I tried JavaScript ID3 Reader and this jsmediatags,js

They both fail to get tags from external websites that don't support HTTP Range feature or they don't allow If-Modified-Since header.

So I edited jsmediatags.js file:
#1977: Removed: xhr.setRequestHeader("If-Modified-Since", "Sat, 01 Jan 1970 00:00:00 GMT");
#1813: Replaced with: var contentLength = null;

What I did ? I forced to download the entire file, which this should be automatically If required headers are not supported on the remote URL's. please fix the fallback ASAP

Example URL: https://crossorigin.me/http://s3.amazonaws.com/static.noisetrade.com/w/8a6a64e5-5c2c-4584-ad46-bba625de301e/hey-tree-streaming.mp3

@aadsm
Copy link
Owner

aadsm commented Feb 10, 2016

This is definitely a bug, it should fetch the entire file when Range is not supported.
Thank you for the heads up and the example, let me investigate.

@aadsm aadsm self-assigned this Feb 10, 2016
@ghost
Copy link
Author

ghost commented Feb 10, 2016

That's great, I'm waiting for your new update,

I have to say something, I'm really impressed by this library, I've tried almost EVERY JavaScript ID3 library out there, you have the best JavaScript ID3 library here. It's even memory-efficient,
way better than JavaScript ID3 Reader (It has memory usage problem.. takes 20MB for every song file).

P.S. I've tried setTagsToReadon m4a song file.

.setTagsToRead(["COMM", "TCON", "WXXX"]) this works.
.setTagsToRead(["comment", "track", "lyrics"]) this doesn't (returns empty tags)

@aadsm
Copy link
Owner

aadsm commented Feb 11, 2016

Thank you so much for your kind words!
Let me create a new issue to track down that one. I might only have time to address this on friday / the weekend.

@aadsm
Copy link
Owner

aadsm commented Feb 13, 2016

I've just fixed both issues and published a new version on npm.

@ghost
Copy link
Author

ghost commented Mar 10, 2016

I don't think the problem is fixed, I'm still not able to get tags from CORS-enabled remote URLs.
Could you please check again ?

I have a proposition for you:
You can get arraybuffer or blob using XHR then pass them to JSMediaTags.

@aadsm aadsm reopened this Mar 14, 2016
@ghost
Copy link
Author

ghost commented Mar 14, 2016

Or do try and catch for headers, inside catch statement do "fetch the entire file" method, that would work too

@aadsm
Copy link
Owner

aadsm commented Apr 29, 2016

Is this still an issue? It works well for me, I've tried it here: https://tonicdev.com/572393dd79f8ad120098dd9e/572393dd79f8ad120098dd9f

@ghost
Copy link
Author

ghost commented May 1, 2016

It still doesn't work, try it in the browser, it will throw "Request header field If-Modified-Since is not allowed by Access-Control-Allow-Headers in preflight response."

Anyway, I've chosen to remove anything related to "download the entire file", which is a bad experience for users, However the bug still exists.

I recommend "try and catch" solution for headers.
Thanks again for this work.

@aadsm
Copy link
Owner

aadsm commented May 1, 2016

When you're not able to whitelist the needed headers on the server you need to disallow their usage:

jsmediatags.Config.setDisallowedXhrHeaders(['If-Modified-Since', 'Range']);

In this particular case it still doesn't work because the request never finishes, maybe crossorigin.me doesn't allow requests greater than 2MB? I'm not sure about the reason but it doesn't timeout so it keeps waiting for more data from the server.
To solve this I added a way to configure the request timeout and default it to 30s.

You're not able to try/catch these XHR errors, you can know about them with the onerror event.

@aadsm aadsm closed this as completed in ca28eb0 May 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant