Resolve Dependency correctly if the target asset is URI-encoded - #401
Conversation
|
I think this should be specific to HTML assets. We don't want to double decode in the case where something has a legit percent symbol in the filename. |
|
I agree on the HTML asset only piece. Let's limit this to the use case. A few more thoughts:
|
bndnsmth
changed the title
|
Thank you for your comments. I'd like to reorganize this patch into the HTML asset, but I can't understand where I should insert the patch into the source code of the HTML asset. Help me! |
|
Hi, if you hop on our slack and send me a message (I'm @shawwn) I can help
you sort through the codebase.
…On Sat, Dec 30, 2017 at 5:35 AM, Susumu Yamazaki ***@***.***> wrote:
Thank you for your comments.
I'd like to reorganize this patch into the HTML asset, but I can't
understand where I should insert the patch into the source code of the HTML
asset. Help me!
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#401 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADo8AajobqtBTKOMGoVL7RvHrOk9WPgks5tFiAQgaJpZM4RMTqh>
.
|
|
Thank you! I sent a Slack message, just now. |
|
I've just pushed PR worked with shawwn. |
|
@devongovett @brandon93s This PR looks good to me now. Any further thoughts? I think the UTF-8 concern is good, but we might want to merge this for now. Supporting at least one Japanese user is better than supporting all hypothetical foreign users, and @zacky1972 has spent a lot of time on this already. |
|
Would be awesome to add a test if possible, but it looks good to me otherwise! |
|
OK! I'd like to add a test. |
add a test code
|
I add a test code. But, I found another problem. This patch assumes encoding of the file system is UTF-8. I'd like to test such systems, but I don't have them. |
|
I call for help by Twitter and Facebook. https://twitter.com/zacky1972/status/949342180430237697
This means: Help us, if you use parcel and encoding of the file system of your PC (Windows or Linux etc.) is not UTF-8! #RetweetMe |
|
I think Node will handle converting from UTF8 and back for us, but it would be good to verify. Some info here: https://nodejs.org/api/fs.html#fs_buffer_api |
|
Oh, really! |
|
That seems true for the encoding of the files, but We may want to take a "try-decode" approach here, continuing with the raw value if decoding fails. |
devongovett
changed the title
|
Let's get this in and deal with any other encoding issues as they come up. Thanks for your work on this @zacky1972! |
|
You're welcome! |
|
I've confirmed it on Windows PC of the Japanese edition, and it works well. |
* Resolve Dependency correctly if the target asset is URI-encoded * Resolve Dependency correctly if the target asset is URI-encoded * revert decodeURIComponent * Resolve Dependency correctry if the target asset is URI-encoded * add a test code
* Resolve Dependency correctly if the target asset is URI-encoded * Resolve Dependency correctly if the target asset is URI-encoded * revert decodeURIComponent * Resolve Dependency correctry if the target asset is URI-encoded * add a test code
4 participants
This patch fixes the bug that parcel can't read assets including encoded URI such as: