Skip to content

Conversation

@icidasset
Copy link
Contributor

Hey @Borewit 👋
I hope you're doing well.

This isn't urgent at all, so for whenever you can get to this.

I noticed a few edge cases with the url resolving, so I figured that it'd be better to make this optional (ie. configurable). In my case specifically, I had a service worker that acted on a specific type of url. What happened was that the tokenizer here cached the resolved url, which normally would be great, but now the service worker was skipped. So this caused an issue in my code, because I don't know about the resolved url in my service worker.

Anyhow long story short, after some thought, I think it's better to make this optional.

@lgtm-com
Copy link

lgtm-com bot commented May 7, 2020

This pull request introduces 24 alerts when merging d5aac5c into 510cdf2 - view on LGTM.com

new alerts:

  • 24 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented May 7, 2020

This pull request introduces 23 alerts when merging 14742e8 into 510cdf2 - view on LGTM.com

new alerts:

  • 23 for Syntax error

@icidasset icidasset force-pushed the client-config branch 2 times, most recently from 76c6836 to 44a25cf Compare May 7, 2020 19:36
@icidasset
Copy link
Contributor Author

Fixed the linting errors and what not.
Reason tests are failing is because of the audio-file testing.

TypeError: NetworkError when attempting to fetch resource.

@Borewit
Copy link
Owner

Borewit commented May 19, 2020

Thanks for your PR @icidasset. I am sorry, I am very busy at the moment.

Reason tests are failing is because of the audio-file testing.

I need to fix that, now the unit tests are in the dark. I had quick look, did not figure it out yet.

@icidasset
Copy link
Contributor Author

No worries, no rush at all! 😉

@Borewit Borewit added dependencies Pull requests that update a dependency file API change enhancement New feature or request and removed dependencies Pull requests that update a dependency file labels Jul 12, 2020
@Borewit
Copy link
Owner

Borewit commented Jul 14, 2020

The resolvedUrl is the URL returned by the HTTP HEAD request.

This URL is then used by the ranged GET request. In case the HTTP HEAD request is not used, the url provided by the user is used. It is not clear to me why this is failing for you. I must also admit that I forgot why I have put the resolvedUrl in there in the first place. I guess it is a kind of follow redirects if the HEAD requests, to prevent following the train twice, or maybe the ranged request fails on the original URL.

@icidasset
Copy link
Contributor Author

Oh yeah for sure, I know why it is there and how it works, because I was the person who added it in the first place 😄 It was indeed added to prevent the following of redirects multiple times. But I noticed that with some services/servers/apis those redirect chains are necessary, hence this PR to make it optional. I hope that clears things up 😉 Let me know if you have any other questions.

PS. Marked in bold the reason it was failing for me.

@Borewit
Copy link
Owner

Borewit commented Jul 17, 2020

Oh yeah for sure, I know why it is there and how it works, because I was the person who added it in the first place.

Ah, okay, LoL. All clear now.

The configuration options need to be documented: #357.

@Borewit Borewit mentioned this pull request Jul 17, 2020
1 task
@Borewit Borewit merged commit 2dcd7df into Borewit:master Jul 17, 2020
@Borewit
Copy link
Owner

Borewit commented Jul 22, 2020

Part of v0.6.0

@icidasset
Copy link
Contributor Author

Thank you @Borewit ! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API change enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants