Skip to content

fix: provide better error message in case of missing +page.svelte - #8478

Merged
Rich-Harris merged 3 commits into
masterfrom
page-error-msg
Jan 13, 2023
Merged

fix: provide better error message in case of missing +page.svelte#8478
Rich-Harris merged 3 commits into
masterfrom
page-error-msg

Conversation

@dummdidumm

Copy link
Copy Markdown
Member

closes #8270

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 pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@changeset-bot

changeset-bot Bot commented Jan 12, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c41478c

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit 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

: null;

if (page_config.ssr) {
if (!branch.at(-1)?.node.component) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not wrapped with DEV because I noticed it's false when running vite dev in sites/kit.svelte.dev, similar to what Rich encountered. I'm wondering if this hints at a bigger problem? Is DEV not working as expected after all?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed an issue so we can have a single place to investigate: #8492

@Rich-Harris

Copy link
Copy Markdown
Member

Is this ssr specific? If you create src/routes/foo/+page.js with export const ssr = false and navigate to /foo, you'll see a blank page, which feels like a bug. I think we probably want to insist on there being a +page.svelte alongside any +page.js or +page.server.js regardless of page config (and we could do that check a single time, in create_manifest_data)

@Rich-Harris

Copy link
Copy Markdown
Member

Hmm, except then again I've written +page(.server).js like this without a +page.svelte file before, and it seems like a reasonable thing to be able to do:

import { redirect } from '@sveltejs/kit';

export function load() {
  throw redirect(308, '/docs/introduction');
}

@dummdidumm

Copy link
Copy Markdown
Member Author

Yes, because of that we can't just have a static check. The linked issue is handled by the implemented check, your edge case isn't thought, but I doubt that someone having only export const ssr = false and no load function or +page.svelte occurs that often that we should try to catch this, too. (theoretically it's possible to catch this case at runtime, too, but would require more code gymnastics than I think it's worth)

@Rich-Harris

Copy link
Copy Markdown
Member

Yeah I had a quick go at adding logic to client.js but couldn't immediately see an easy way to make it work, so I'm happy to leave that as a TODO for another day

Comment thread packages/kit/src/runtime/server/page/render.js Outdated
Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
@Rich-Harris
Rich-Harris merged commit f09ad43 into master Jan 13, 2023
@Rich-Harris
Rich-Harris deleted the page-error-msg branch January 13, 2023 20:07
@github-actions github-actions Bot mentioned this pull request Jan 13, 2023
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.

500 internal error using form actions from different directory when calling fail() (node.component is not a function)

3 participants