avoid double-encoding file:/// urls

[marcelblum]

#902 introduced a breaking change in that the file loader now assumes a given URL is unencoded. This is probably fine in most use cases but when loading local files using the file:/ scheme the url is already encoded.

(A more complete solution might be to add a flag somewhere so the user can specify if they know the url being passed to Tone is already encoded, to avoid double-encoding. But I figured a simple check for startsWith("file:/") is best for now to avoid an overly-complex solution unless it affects other users in more exotic scenarios other than just file:///.)

[danferns]

I'm very sorry for not thinking about that, but thank you so much for the fix! :)

On looking up a bit and with some experimentation, I found this test to check if a URL is already encoded.

if (decodeURIComponent(string) === string) {
    // we can encode it
}

It will skip URLs that do not need encoding. And will also encode file:/// URLs that weren't encoded!

Unless you disagree, it would be cool if you could include this!

[marcelblum]

@dannywritescode That sounds good, if (decodeURIComponent(string) === string) seems like a reasonably accurate way to screen for already-encoded urls (well, there are some very edge cases would fail, but probably much less likely to encounter those cases than for some Tone.js developer to want to pass an encoded url in certain scenarios). That said, that alone does not avoid issues with file: urls specifically; all valid file: urls are already encoded as per the spec as I understand it, yet they might not contain any characters requiring percent-encoding, so could easily come up as unencoded with the if (decodeURIComponent(string) === string) test but then due to the unique rules of the file URI scheme get erroneously mangled by the brute force of
location.pathname = (location.pathname + location.hash).split("/").map(encodeURIComponent).join("/");
So I'm thinking best to keep an explicit check for file:/ while also adding a check for decodeURIComponent(string) === string.

[marcelblum]

Added a provision for relative path local file:/ urls. If you put a relative path into the href of an a node object, the resulting node.href returns the full already percent-encoded path.

[JeffreyCA]

Hi can you confirm if this fixes the issue described in #1039 with foreign characters?

[marcelblum]

@JeffreyCA I just tested and it works with this fix but only if you encode the url yourself before passing it to Tone. I'm going to add another check that should make pre-encoding unnecessary. I think the issue is that in many cases the browser automatically encodes a url when you do anchorElement.href=urlString. The present code was taking the string returned from that href property and then double-encoding it in some cases. I'm just going to add another check for an already-encoded url deeper in the process to avoid this issue.

[tambien]

What do you all think of removing all of the url handling stuff altogether? I think there used to be more of a reason to enable multiple file extensions, but i think at this point maybe it's not as useful a feature and is just getting in the way more than it's helping?

If we removed all of the URL modifying code, we could just require that the user escape the url string they pass

@marcelblum what do you think? i appreciate all of the work that you've done fixing this feature

[marcelblum]

@tambien I'd be fine with that, TBH prior to #902 in my own use of Tone.js I had gotten into the habit of always pre-encoding all urls I passed to Tone functions. And the current url handling is certainly getting a tad messy. That said, it seems like a really common issue for other Tone.js users to run into trying to pass filenames like "C#2.wav" and getting errors because they expect Tone to handle url encoding automatically, and it is a nice-to-have if Tone handled that automatically. Just trying to make everyone happy 😃

[JeffreyCA]

Me too, I think supporting both scenarios with a flag mechanism like @marcelblum mentioned would work well too.
In any case we should add documentation that says URLs should/shouldn't be pre-encoded.

[danferns]

I agree, the current URL handling has quite some side-effects that I didn't see. I think the problem is that it encodes all characters by default, and then excludes those (like /) that should be kept as is.

Since we only really need to encode the # character, could we use url.replace("#", "%23") instead?

The # symbol in a URL is used to scroll to a specific part of a page (example). If it doesn't have any special meaning in file URLs, then we will be able to support filenames like C#.wav without any side effects because we are replacing only this one character.

I understand if we don't want any URL handling at all though, as that is more transparent.

I think there is value in supporting the hash symbol out of the box (sharps seem to be more commonly used in file names, as if it's an informal standard, and some DAW software like FL Studio also use sharps when exporting notes).

I'm also happy with the idea of using a flag!

[slocka]

I have raised a new issue #1097 related to the same problem. I think the trick of decodeURIComponent(href) === href would work for my case but I am really not sure if that covers all the cases. My 2 cents would be to try to keep it simple and avoid doing any transformation to the URL and just let it be the string provided by the user. It would be much more flexible in the end to let the application decide how to transform the string. And in case there is common transformation that needs to be done (C#.mp3 for example), maybe it could be exported as a separate util to transform the string instead of part of the core of the ToneAudioBuffer class?