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

Can't load URL in Tone.Player if URL is already pre-encoded. #1097

Open
slocka opened this issue Jul 6, 2022 · 1 comment · May be fixed by #1169
Open

Can't load URL in Tone.Player if URL is already pre-encoded. #1097

slocka opened this issue Jul 6, 2022 · 1 comment · May be fixed by #1169

Comments

@slocka
Copy link

slocka commented Jul 6, 2022

Describe the bug

In the @next version of tone , calling player.load(url) with a URL that is already encoded is likely to fail because of a fetch error. Tone js tries to encode the content of the pathname but if this one was already encoded when given to the load function, the resulting string given to the fetch function ends up encoded twice and therefore the fetch request fails.

By always encoding the provided URL, tone is taking the risk of breaking URLs that do not need/want extra encoding as encodeURIComponent will encode characters that were used to escape the original characters.
i.e.:

  • encodeURIComponent("/myfile") => %2Fmyfile
  • encodeURIComponent(encodeURIComponent("/myfile")) => '%252Fmyfile' (Here % is encoded to %25)

This makes it impossible to load files coming from firebase storage for example since the URL to the file contains the char % which is automatically encoded by tone which then tries to fetch a different URL resulting in a 404. (See codesandbox example).

IMO tone JS should not do any type of specific URL encoding logic as this can easily be done at the application level if needed and it breaks URLs that don't need to be encoded. If we want to still keep it because it's handy, it could be a better idea to move it to a separate util or to at least introduce an option to bypass the encoding when it's doing more harm than good.

To Reproduce

This is because the URL we provided was:
https://firebasestorage.googleapis.com/v0/b/test-7128f.appspot.com/o/MyFolder%2FmusicInC%23ornot.mp3?alt=media&token=dcc02d0e-d5db-4a93-976f-5d37c4d60e71

but the URL used to fetch was changed to:
https://firebasestorage.googleapis.com/v0/b/test-7128f.appspot.com/o/MyFolder%252FmusicInC%2523ornot.mp3?alt=media&token=dcc02d0e-d5db-4a93-976f-5d37c4d60e71
`

Expected behavior
The request succeeds and the file loads correctly using the original URL provided to the .load function.

What I've tried

I looked for the breaking change since it works fine in the @latest version of the package and it seems that this is the change that introduced the issue: #902

Additional context
Tested with 14.8.40 but this is not specific to that version. Version 14.7.77 works just fine in the same scenario (try to change the tone dependency to 14.7.77 in codesandbox and it will then load correctly.)

@ScottMorse
Copy link

My PR #1169 addresses this by bypassing encoding if it can be determined that the given URI component is already encoded.

Here is a fixed fork of your sandbox that demonstrates that your file will load correctly with this implementation: https://codesandbox.io/s/tone-next-url-encode-issue-forked-5nn9go.

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

Successfully merging a pull request may close this issue.

2 participants