search

zmxv / react-native-sound

React Native module for playing sound clips
MIT License
2.78k stars 747 forks source link

remove buggy url encoding #830

Open KnowYourLines opened 11 months ago

KnowYourLines commented 11 months ago

Fixes https://github.com/zmxv/react-native-sound/issues/500

Reda1337 commented 11 months ago

How much will it take to merge this fix? it worked for me locally but i would love if it's available in the package.

RomualdPercereau commented 7 months ago

Could you please give some example of context working after and before? Is it still working on with filename with spaces?

RomualdPercereau commented 7 months ago

This look like to be the opposite of this fix https://github.com/zmxv/react-native-sound/commit/3fe5480fce935e888d5089d94a191c7c7e3aa190

@KnowYourLines @Reda1337

KnowYourLines commented 6 months ago

This look like to be the opposite of this fix 3fe5480

@KnowYourLines @Reda1337

Yes that fix causes this problem. https://github.com/zmxv/react-native-sound/pull/628#issuecomment-1179426688 from that PR points this out.

Frankly, I don't believe URI encoding should be done by this library to begin with as there are very likely more effective libraries that accomplish URI encoding and would allow encoded URIs to be passed to this library.

It would make more sense for the users of this library to be responsible for passing an appropriate URI to this library rather than have default URI encoding in this library that enables some inputs to work but prevents others from working.

The current URI encoding in this library seems like a premature optimization since it arguably causes as many if not more more problems than it solves.

@RomualdPercereau

OlivierCo commented 2 months ago

Can we merge it?

OlivierCo commented 2 months ago

@RomualdPercereau I had to revert the change made in this 3fe5480. and now it works perfectly fine. So Could we merge please