[fix] decode path only once to fix loading unicode routes #2167

noahsalvi · 2021-08-11T14:08:54Z

Before submitting the PR, please make sure you do the following

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check
running check returns this.. but i don't think thats due to my changes

Scope: all 12 workspace projects
packages/kit check$ tsc && svelte-check --ignore "src/core/make_package/test"
[3 lines collapsed]
│ Error while loading config
│ TypeError: Cannot read property 'compilerOptions' of undefined
│     at ConfigLoader.loadConfig (/Users/noahsalvi/workspace/kit/node_modules/.pnpm/[email protected][email protected]/node_m
│     at async ConfigLoader.loadAndCacheConfig (/Users/noahsalvi/workspace/kit/node_modules/.pnpm/[email protected]_svelte
│     at async /Users/noahsalvi/workspace/kit/node_modules/.pnpm/[email protected][email protected]/node_modules/svelte-check
│     at async Promise.all (index 2)
│     at async ConfigLoader.loadConfigs (/Users/noahsalvi/workspace/kit/node_modules/.pnpm/[email protected][email protected]
│     at async createLanguageService (/Users/noahsalvi/workspace/kit/node_modules/.pnpm/[email protected][email protected]/no
│ ====================================
│ svelte-check found 0 errors, 0 warnings, and 0 hints
└─ Done in 11.1s
packages/adapter-static check$ tsc
└─ Done in 1.7s
packages/create-svelte check$ tsc
└─ Done in 2.6s

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

changeset-bot · 2021-08-11T14:08:57Z

🦋 Changeset detected

Latest commit: 50dfe21

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@sveltejs/adapter-node	Patch
@sveltejs/adapter-vercel	Patch
@sveltejs/adapter-cloudflare-workers	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

bluwy

I think the changes in pnpm-lock.yaml should be removed in this PR.

noahsalvi · 2021-08-11T14:48:23Z

done.

benmccann · 2021-08-11T16:19:11Z

It looks like this is handled inconsistently today. adapter-vercel, adapter-cloudflare-workers would also need to be updated as they're currently decoding the path. (adapter-netlify and architect/sveltekit-adapter do not while it's not relevant for adapter-static)

CC @pluvial for svelte-adapter-deno and @jthegedus for svelte-adapter-firebase which also parse the URL today

We should also figure out how this interacts with handle. I think we are returning the decoded version there today and perhaps we should not be to be consistent with the change you are making here. Express does not decode the path so returning the un-decoded path would make it more consistent and easier to interact with existing middlewares expressjs/expressjs.com#1281.

I think we should not decode the path in the adapters for the reasons mentioned at expressjs/expressjs.com#1281 (comment). And clearly we don't want to do it twice as we are now. Plus it would be more consistent with Express. Most important is probably that we do it consistently and test and document the expectations.

benmccann · 2021-08-11T16:35:44Z

@noahsalvi would you be able to update adapter-vercel and adapter-cloudflare-workers as you have here?

noahsalvi · 2021-08-11T16:37:28Z

@noahsalvi would you be able to update adapter-vercel and adapter-cloudflare-workers as you have here?

Sure 👍

noahsalvi · 2021-08-11T16:40:54Z

btw i thought that the node adapter is using polka. Shouldn't that be the place to look? Or does it not matter due to express and polka sharing the same api?
@benmccann

benmccann · 2021-08-11T16:45:45Z

Yeah, I just mentioned Express because it is more popular. But I just tested and confirmed that Polka works the same as Express in this manner

.changeset/perfect-zoos-fix.md

noahsalvi · 2021-08-11T17:21:40Z

packages/adapter-cloudflare-workers/files/entry.js

-	// fall back to an app route
-	const request = event.request;
-	const request_url = new URL(request.url);
+	const parsedURL = new URL(request.url);


CFW request doesn't expose the usual props. That's why I did it differently for this adapter. Doc

benmccann · 2021-08-11T17:49:52Z

packages/adapter-node/src/server.js

-				path: parsed.pathname,
-				query: parsed.searchParams,
-				rawBody: body
+				path: req.path,


It's not clear to me that this PR is actually changing anything

I just created an index.js with the source code:
console.log(new URL('http://localhost/über-uns').pathname)

It printed:

/%C3%BCber-uns

If I change the code to console.log(new URL('http://localhost/%C3%BCber-uns').pathname) it still prints the same thing

So it's actually not clear to me that this PR is changing anything. Am I missing something? Or can you explain what was happening before vs what's happening now?

Well at the moment the render function is receiving, as the path value, the encoded string /%C3%BCber-uns.

And now after changing it to take the original path from the req object without transforming and encoding it with URL class, the value that it receives is /über-uns

I think that the thing you're missing is, that my change for the adapter node, does not wrap the path in a url object, that encodes the string.

Edit:
To observe this, you can clone my repro and modify the server.js file in node_modules to print out req.path and parsed.pathname

benmccann · 2021-08-11T19:04:06Z

Gahhh!!! This is Polka. I tested with @latest but it changed behavior in @next. I sent #2171 as a better fix. I think this should only fix adapter-node after all. It was a bit of a round-about path to figuring out what was going on. Thanks for the hlpe with this one

noahsalvi · 2021-08-11T20:47:35Z

are you sure that your fix works?
Maybe we aren't talking about the same problem, because I tried your fix and it did not work for my repro project.

Just to clarify, routes with unicode characters work on my computer only when the render function gets a none URI encoded path. Using the polka parser or the URL class to get pathname leads to it not working.

await render({
	method: req.method,
	headers: req.headers, // TODO: what about repeated headers, i.e. string[]
	path: '/über-uns' works but '/%C3%BCber-uns' doesn't,
	query: parsed.searchParams,
	rawBody: body
});

benmccann · 2021-08-11T22:55:33Z

Thanks for testing @noahsalvi. I updated the PR and added a new test for adapter-node. Can you test again?

noahsalvi · 2021-08-12T15:53:04Z

Thanks for testing @noahsalvi. I updated the PR and added a new test for adapter-node. Can you test again?

I just tested it and the test you wrote works, but it does not test the problem that i'm facing. I also tried linking the package to my repro repo and it did not change the outcome.

test('route with umlaut returns page', async () => {
	const server = await startServer({
		render: (incoming) => {
			if (incoming.path === '/über-uns') {
				return {
					status: 200,
					body: incoming.path
				};
			}
		}
	});
	const res = await fetch(`http://localhost:${PORT}/über-uns`);
	assert.ok(res.ok);
	server.server.close();
});

^ This would be testing this problem.

(Edit: I've added the test to this branch)

benmccann · 2021-08-12T16:12:54Z

I just tried linking my PR against your repo and it did work

You unit test isn't correct. The browser will send an encoded path so you can't check the encoded incoming.path against a decoded path. But the router will decode the path when looking for a matching rout and will find the correct path and so I don't get a 404 when running against the full server

noahsalvi · 2021-08-12T17:02:16Z

hmmm weird....

Just to confirm, what was the result you got from running my test?

remove url parsing in server.js

76671a7

bluwy reviewed Aug 11, 2021

View reviewed changes

noahsalvi added 2 commits August 11, 2021 16:42

revert pnpm-lock for PR for issues/2166

8e3232e

revert pnpm-lock for PR issues/2167 without formatting it

391f367

benmccann added the bug Something isn't working label Aug 11, 2021

benmccann changed the title ~~remove url parsing in server.js~~ [fix] remove url parsing from adapter-node Aug 11, 2021

remove url parsing from adapter-vercel

7402ee0

benmccann reviewed Aug 11, 2021

View reviewed changes

.changeset/perfect-zoos-fix.md Show resolved Hide resolved

benmccann reviewed Aug 11, 2021

View reviewed changes

.changeset/perfect-zoos-fix.md Outdated Show resolved Hide resolved

benmccann changed the title ~~[fix] remove url parsing from adapter-node~~ [fix] decode path only once to fix loading unicode routes Aug 11, 2021

decode path before rendering for adapter-cloudflare-workers

50dfe21

noahsalvi commented Aug 11, 2021

View reviewed changes

benmccann reviewed Aug 11, 2021

View reviewed changes

benmccann closed this Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] decode path only once to fix loading unicode routes #2167

[fix] decode path only once to fix loading unicode routes #2167

noahsalvi commented Aug 11, 2021 •

edited

Loading

changeset-bot bot commented Aug 11, 2021 •

edited

Loading

bluwy left a comment

noahsalvi commented Aug 11, 2021

benmccann commented Aug 11, 2021 •

edited

Loading

benmccann commented Aug 11, 2021

noahsalvi commented Aug 11, 2021

noahsalvi commented Aug 11, 2021

benmccann commented Aug 11, 2021

noahsalvi Aug 11, 2021

benmccann Aug 11, 2021

noahsalvi Aug 11, 2021 •

edited

Loading

benmccann commented Aug 11, 2021

noahsalvi commented Aug 11, 2021

benmccann commented Aug 11, 2021

noahsalvi commented Aug 12, 2021 •

edited

Loading

benmccann commented Aug 12, 2021

noahsalvi commented Aug 12, 2021

[fix] decode path only once to fix loading unicode routes #2167

[fix] decode path only once to fix loading unicode routes #2167

Conversation

noahsalvi commented Aug 11, 2021 • edited Loading

Before submitting the PR, please make sure you do the following

Tests

Changesets

changeset-bot bot commented Aug 11, 2021 • edited Loading

🦋 Changeset detected

bluwy left a comment

Choose a reason for hiding this comment

noahsalvi commented Aug 11, 2021

benmccann commented Aug 11, 2021 • edited Loading

benmccann commented Aug 11, 2021

noahsalvi commented Aug 11, 2021

noahsalvi commented Aug 11, 2021

benmccann commented Aug 11, 2021

noahsalvi Aug 11, 2021

Choose a reason for hiding this comment

benmccann Aug 11, 2021

Choose a reason for hiding this comment

noahsalvi Aug 11, 2021 • edited Loading

Choose a reason for hiding this comment

benmccann commented Aug 11, 2021

noahsalvi commented Aug 11, 2021

benmccann commented Aug 11, 2021

noahsalvi commented Aug 12, 2021 • edited Loading

benmccann commented Aug 12, 2021

noahsalvi commented Aug 12, 2021

noahsalvi commented Aug 11, 2021 •

edited

Loading

changeset-bot bot commented Aug 11, 2021 •

edited

Loading

benmccann commented Aug 11, 2021 •

edited

Loading

noahsalvi Aug 11, 2021 •

edited

Loading

noahsalvi commented Aug 12, 2021 •

edited

Loading