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 doc 404s #4084

Closed
wants to merge 3 commits into from
Closed

handle doc 404s #4084

wants to merge 3 commits into from

Conversation

Rich-Harris
Copy link
Member

Received a report that a link to /docs/typescript was 500ing rather than 404ing. This fixes it. An alternative/additional fix would be to set up redirects every time we make a change to the structure of the docs, but I think that would be a bad use of time at least until 1.0

Please don't delete this checklist! 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

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
Copy link

changeset-bot bot commented Feb 23, 2022

⚠️ No Changeset found

Latest commit: c3aad06

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@Rich-Harris
Copy link
Member Author

Gah, it doesn't fix it. If you try to hit a non-prerendered page, it will fall back to the serverless function, which attempts to read a non-existent file from the filesystem. Might have to do a gross workaround

@Conduitry
Copy link
Member

There was an issue opened for this recently, which I was having trouble finding. Turns out because it was opened in the Svelte repo. I've moved it here: #4086.

@Rich-Harris
Copy link
Member Author

So... we could certainly work around this, but it wouldn't be that pretty, and it would just be masking the real issue which is that the endpoint shouldn't be deployed anyway.

I think the solution is to add export const prerendered to endpoints, and exclude prerendered endpoints from the generated route manifests.

@benmccann
Copy link
Member

Guess I should have read the comments before approving 😁

Are there any non-prerendered pages on the SvelteKit site? Maybe you could use adapter-static with the fallback option?

Or you might be able to do something like:

	if (process.env.NODE_ENV === 'production' && !$prerendering) {
		return { status: 404 };
	}

@Rich-Harris
Copy link
Member Author

adapter-static is an option, but only because we happen not to have any dynamic pages yet — i'd rather fix the underlying issue. deploying a 'real' app also means we get custom error pages instead of the minimalist Vercel black-on-white status code

@Rich-Harris
Copy link
Member Author

have gone with the temporary workaround since prerendered endpoints might be a bit of a rabbit hole (e.g. i think we would want to do #4112 first)

@Rich-Harris
Copy link
Member Author

@Rich-Harris
Copy link
Member Author

It should be loading /docs.json from prerendered files, but isn't 🤔

@Rich-Harris
Copy link
Member Author

closing this in favour of #4093

@Rich-Harris Rich-Harris deleted the docs-404 branch April 22, 2022 16:15
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.

None yet

3 participants