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

Clashing endpoint-powered pages break route sorting #4038

Closed
mrkishi opened this issue Feb 21, 2022 · 4 comments · Fixed by #4203
Closed

Clashing endpoint-powered pages break route sorting #4038

mrkishi opened this issue Feb 21, 2022 · 4 comments · Fixed by #4203
Labels
bug Something isn't working
Milestone

Comments

@mrkishi
Copy link
Member

mrkishi commented Feb 21, 2022

Describe the bug

When multiple endpoint-powered pages have a clashing route path, both client- and server-side routers break, not following sorting rules.

Given:

/[id].js
/[id].svelte
/[slug].js
/[slug].svelte

A client-side navigation to any matching route renders [id].svelte with undefined props, without hitting the endpoint.

A full-page navigation does hit the endpoints and falls through them, but you get back their JSON payload instead of the rendered page.

Reproduction

Minimal reproduction available here.

Logs

No response

System Info

System:
    OS: Windows 10 10.0.19044
    CPU: (4) x64 Intel(R) Core(TM) i5-3570 CPU @ 3.40GHz
    Memory: 1.44 GB / 7.88 GB
  Binaries:
    Node: 17.5.0 - ~\scoop\apps\nodejs\current\node.EXE
    npm: 8.4.0 - ~\scoop\apps\nodejs\current\bin\npm.CMD
  Browsers:
    Chrome: 98.0.4758.102
    Edge: Spartan (44.19041.1266.0), Chromium (98.0.1108.56)
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.17
    @sveltejs/kit: next => 1.0.0-next.279
    svelte: ^3.44.0 => 3.46.4

Severity

serious, but I can work around it

Additional Information

No response

@Rich-Harris Rich-Harris added the bug Something isn't working label Feb 21, 2022
@benmccann benmccann added this to the 1.0 milestone Feb 22, 2022
@isaacfranco
Copy link
Contributor

isaacfranco commented Feb 22, 2022

I saw that the strategy to associate a page route with it's shadow endpoint requires that they are just after the other, after route sorting.

The problem is that when we have multiple endpoint-powered pages on the same folder, all it's endpoints stay before all the pages, and the association strategy doesn't work.

On packages/kit/src/core/create_manifest_data/index.js there is a comparator function that does the route sorting, and there is a code, on the later part of it, that put pages after endpoints:

	if (a.is_page !== b.is_page) {
		return a.is_page ? 1 : -1;
	}

if we remove that, the bug is gone, but I'm feeling that it will not respect the priorities especified on the documentation anymore.

Someone with any thoughts?

@isaacfranco
Copy link
Contributor

I can try to keep the sort comparator intact, but change this:

while (i--) {
		const route = routes[i];
		const prev = routes[i - 1];

		if (prev && prev.key === route.key) {
			if (prev.type !== 'endpoint' || route.type !== 'page') {
				const relative = path.relative(cwd, path.resolve(config.kit.files.routes, prev.key));
				throw new Error(`Duplicate route files: ${relative}`);
			}

			route.shadow = prev.file;
			routes.splice(--i, 1);
		}
	}

... to someting that doesn't require page routes and its shadow-endpoints to be together after sorting.

@ITwrx
Copy link

ITwrx commented Mar 1, 2022

is

index.svelte
index.js
[root_page].svelte
[root_page].js

a clash? If so, how are we supposed to specify both index only and anything else at same level without it being a clash? My app is serving the endpoint's json response instead of the page right now.

described more in depth here: #4162

btw, this comment is largely intended to make sure my exact situation is known, as opposed to having my question answered right this second. :)

@kvetoslavnovak
Copy link
Contributor

I van confirm to encounter the very same problem using endpoints. In here above described case the endpoints seem not to follow the sorting rules.

Rich-Harris added a commit that referenced this issue Mar 4, 2022
Rich-Harris added a commit that referenced this issue Mar 4, 2022
…igation (#4203)

* add failing test from #4139

* fix #4038

* changeset

* add comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants