urlParsed removes trailing slash

[openscript]

Description

When the urlOriginal is /en/index.pageContext.json?asd=asd and in the address bar is displayed /en/?asd=asd the urlPathname is /en. When the urlOriginal is /en/?asd=asd the urlPathname is /en/.

This behavior is not influenced by disableUrlNormalization: true.

In my opinion the urlPathname should be /en/ when the urlOriginal is /en/index.pageContext.json?asd=asd.

[openscript]

I would be happy to create a PR for this. Probably this lines need to be changed:

vike/vike/node/runtime/renderPage/handlePageContextRequestUrl.ts

Lines 25 to 36 in 6dedbfb

	function removePageContextUrlSuffix(url: string): string {
	const urlParsed = parseUrl(url, baseServer)
	// We cannot use `urlParsed.pathname` because it would break the `urlParsed.pathnameOriginal` value of subsequent `parseUrl()` calls.
	const { origin, pathnameOriginal, searchOriginal, hashOriginal } = urlParsed
	assert(doNotCreateExtraDirectory === false)
	const urlSuffix = `/index${pageContextJsonFileExtension}`
	assert(pathnameOriginal.endsWith(urlSuffix), { url })
	let pathnameModified = slice(pathnameOriginal, 0, -1 * urlSuffix.length)
	if (pathnameModified === '') pathnameModified = '/'
	assert(url === `${origin || ''}${pathnameOriginal}${searchOriginal || ''}${hashOriginal || ''}`, { url })
	return `${origin || ''}${pathnameModified}${searchOriginal || ''}${hashOriginal || ''}`
	}

Are there unit tests for this? Where would you add a unit test?

[brillout]

I agree and PR welcome 👍

I also agree that'd be good to add some Vitest test for it and create a file vike/vike/node/runtime/renderPage/removePageContextUrlSuffix.spec.ts. Let's export removePageContextUrlSuffix() and after the PR is merged I'll move removePageContextUrlSuffix() in its own file. (Let's move it to its own file later so it's easier for me to read the diffs you made to removePageContextUrlSuffix.)

[openscript]

Inside removePageContextUrlSuffix() we cannot know if it was originally /en/?asd=asd or /en?asd=asd. Should we read Vikes option trailingSlash and act according to that? If so, should trailingSlash be a param in removePageContextUrlSuffix() or for parseUrl()?

[brillout]

Yes, that's an issue: there is no way to know whether /en/index.pageContext.json is for a /en/ or /en URL. I'm thinking a trick could be to use /en//index.pageContext.json if the URL is /en/?

Inside removePageContextUrlSuffix() we cannot know if it was originally /en/?asd=asd or /en?asd=asd. Should we read Vikes option trailingSlash and act according to that? If so, should trailingSlash be a param in removePageContextUrlSuffix() or for parseUrl()?

That would be a brittle solution as it would fail for /en URLs then.

[brillout]

Ah, actually more natural: /en/index.pageContext.json/. Much clearer I think.

[openscript]

Where would the trailing slash /en/index.pageContext.json/ be added?

[brillout]

See `vike/shared/getPageContextRequestUrl.ts`

…

On Thu, May 2, 2024 at 10:45 AM Robin ***@***.***> wrote: Where would the trailing slash /en/index.pageContext.json/ being added? — Reply to this email directly, view it on GitHub <#1629 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHVQRRO7N33WP4LLZXSEWDZAH4MHAVCNFSM6AAAAABHDBD3OOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBZHEZDCMRRGM> . You are receiving this because you commented.Message ID: ***@***.***>

[brillout]

Closing as it doesn't seem fixable, see #1632 (comment).

@openscript Let me know whether this is a blocker for you. Otherwise I'll move on and focus on other priorities.

[openscript]

Thank you @brillout. Even though a nice way to handle this, I'm happy that we collaborated.

It's not a blocker as I can handle it case by case in my implementation.