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

handle accented characters etc #11

Merged
merged 1 commit into from
Sep 3, 2018
Merged

Conversation

Rich-Harris
Copy link
Contributor

At present, a file like public/fünke.txt doesn't get served if you hit /fünke, because req.path is /f%C3%BCnke. This fixes it in both dev and prod mode.

@lukeed
Copy link
Owner

lukeed commented Sep 3, 2018

For prod, I think we should just be able encode everything from the glob results, right? Node.js will give us a req.url that's already encoded, so the lookup will pass.

Then for dev, would decode incoming request, which is fine to take the penalty there.

@Rich-Harris
Copy link
Contributor Author

You've lost me — I thought that's what I'd done in this PR, no?!

@lukeed
Copy link
Owner

lukeed commented Sep 3, 2018

Yeah, definitely! I'm just trying to avoid the decodes on every production request. Node will escape funky characters on their way in.

So I think we can be cheeky and just encode into the cache only, since that's how req.url will already be formed.

@Rich-Harris
Copy link
Contributor Author

I thought that's what I'm doing by populating FILES with encoded filenames on line 75?

@lukeed
Copy link
Owner

lukeed commented Sep 3, 2018

Well, I feel foolish. Went back to the diff and saw you did exactly what I was describing. I swear the first time around I saw that you were encoding and decoding for prod.

🙈

I've been doing PWA scaffolds for too long. Will merge and release patch in the morning.

Thanks!

@lukeed lukeed merged commit 250f296 into lukeed:master Sep 3, 2018
@lukeed
Copy link
Owner

lukeed commented Sep 3, 2018

So I'm running this locally with the '/fünke.txt' example you gave. The --dev works – prod has some issues though:

// this is how the "'/fünke.txt'" request comes into Node.js
"/f%C3%BCnke.txt"

// Trying to match that for FILES cache key

encodeURI('/fünke.txt')
'/fu%CC%88nke.txt'

encodeURIComponent('/fünke.txt')
'%2Ffu%CC%88nke.txt'

escape('/fünke.txt')
'/fu%u0308nke.txt'

querystring.escape('fünke.txt')
'fu%CC%88nke.txt'

Likely has to do with the annoying details of unicode that I don't understand... something something multiple codes per character.

Am I missing a possibility? I know decoding works, but I'd like to try paying the toll upfront rather than per request, if possible.

@Rich-Harris
Copy link
Contributor Author

Huh, that's weird... it was working for me last night. Will try and remember what I did.

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 this pull request may close these issues.

2 participants