-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
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:
|
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. |
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
This patch fixes the bug that parcel can't read assets including encoded URI such as: