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

Build process fails after attempting to prerender auth endpoints #11083

Closed
austerj opened this issue Nov 19, 2023 · 22 comments · Fixed by #11405
Closed

Build process fails after attempting to prerender auth endpoints #11083

austerj opened this issue Nov 19, 2023 · 22 comments · Fixed by #11405
Labels
bug Something isn't working
Milestone

Comments

@austerj
Copy link

austerj commented Nov 19, 2023

Describe the bug

I'm working on a PWA that prerenders the root page endpoint located inside an (app) layout group (so the service worker has access to the app shell) with a fetch in (app)/+layout.ts (which is NOT prerendered) that authenticates the user and redirects to a sign-in page at run-time.

This somewhat hacky setup was producing the desired behavior until version 1.27.4 - namely a prerendered index.html app shell that attempts authentication (along with fetching various cacheable assets) only at run-time and makes related data from the parent (app) layout load function accessible to subsequent load functions inside the group.

After version 1.27.4, the build process errors out because it now seems to attempt to authenticate (and redirect) at build time:

node:internal/event_target:1033
  process.nextTick(() => { throw err; });
                           ^
Error: 404 /api/validate (fetched from /)
To suppress or handle this error, implement `handleHttpError` in https://kit.svelte.dev/docs/configuration#prerender

Reviewing the build output, the index.html is now referencing the sign-in page redirected to on failed auth attempts. If I explicitly add export const prerender = false to this endpoint, the error instead becomes

Error: /api/(auth)/validate is not prerenderable

The errors looks similar to #11031 and likewise seems related to the changes introduced here - the issue started in version 1.27.4 and downgrading to 1.27.3 fixes it. It still persists in version 1.27.6 after related issues with fully-static sites seems to have been closed.

Reproduction

Unfortunately a simple reproduction is not feasible due to the dependencies on auth etc.; hopefully the info provided + related issues are enough for a start. If I manage to narrow things down to where a reproduction is feasible I'll post an update.

Logs

No response

System Info

System:
  OS: Linux 6.2 Ubuntu 22.04.3 LTS 22.04.3 LTS (Jammy Jellyfish)
  CPU: (32) x64 AMD Ryzen 9 5950X 16-Core Processor
  Memory: 14.34 GB / 62.71 GB
  Container: Yes
  Shell: 5.1.16 - /bin/bash
Binaries:
  Node: 18.18.0 - ~/.nvm/versions/node/v18.18.0/bin/node
  npm: 9.8.1 - ~/.nvm/versions/node/v18.18.0/bin/npm
  pnpm: 8.9.0 - ~/.nvm/versions/node/v18.18.0/bin/pnpm
npmPackages:
  @sveltejs/adapter-vercel: ^3.0.3 => 3.0.3 
  @sveltejs/kit: ^1.27.6 => 1.27.6 
  svelte: ^4.2.3 => 4.2.3 
  vite: ^4.5.0 => 4.5.0

Severity

blocking an upgrade

Additional Information

No response

@gyurielf
Copy link
Contributor

I ran into as well, but I use prerender="auto" for generating all of the entry files for the cdn (we use it like SPA).
Now constantly generates just redirects into the files.

@eltigerchino
Copy link
Member

eltigerchino commented Nov 20, 2023

Hi, thank you both for reporting this issue. I want to apologise for causing this change that has affected your development.

I tried reproducing the issue based on both your descriptions and what I think could be the cause but was not successful. It will be very helpful if you can share the full build logs from the moment of running npm run build up until the process terminates.

Here is the SPA reproduction that I created to try to reproduce the issue.

I ran into as well, but I use prerender="auto" for generating all of the entry files for the cdn (we use it like SPA).
Now constantly generates just redirects into the files.

It's worth noting that with prerender set to true or 'auto', your load functions will run during prerendering. If you have code that you don't want running during the build, you will need to conditionally check the building environment variable and not execute the code.
https://kit.svelte.dev/docs/modules#$app-environment-building

@gyurielf
Copy link
Contributor

gyurielf commented Nov 20, 2023

Hello there @eltigerchino !

I appreciate your answer.

For adapter-static, the trailingSlash = 'always' comes onto the table.
You can see the actual build output:
https://stackblitz.com/edit/sveltejs-kit-template-default-mt72gz

If you downgrade to 1.27.3 and build it, you can compare the .html entry point outputs.

@austerj
Copy link
Author

austerj commented Nov 20, 2023

Thanks a ton for taking the time to setup the repo @eltigerchino, really appreciate it (and you managed to do a better job of simplifying the setup than I could)!

I had a look and it seems like the build process does not prerender anything (.sveltekit/output has no prerendered folder after npm run build).

By replacing +server.ts with +page.ts with the only content being export const prerender = true;, I am now getting the build to attempt to prerender the index.html file (and failing with the error mentioned initially + the prerendered index.html is referencing the /signin route).

Link to fork

@austerj
Copy link
Author

austerj commented Nov 20, 2023

Also here another fork (restructured slightly to resemble my project) which successfully prerenders the index.html app shell in version 1.27.3 while redirecting to the sign-in at runtime (npm run preview).

@eltigerchino
Copy link
Member

For adapter-static, the trailingSlash = 'always' comes onto the table. You can see the actual build output: stackblitz.com/edit/sveltejs-kit-template-default-mt72gz

If you downgrade to 1.27.3 and build it, you can compare the .html entry point outputs.

The redirects are technically the correct outcome here. Since the trailingSlash is set to 'always', you should be redirecting to URLs with a trailing slash to avoid another redirect. I've done so in the repro below.
https://stackblitz.com/edit/sveltejs-kit-template-default-q9l2pa?file=build%2Fsignin%2Findex.html

Incidentally, the behaviour that your app is trying to produce (the same SPA setup for every HTML file) is exactly what was meant to be fixed. When prerender is set to true or 'auto', the result should always be the same as navigating to the page in the browser and saving the resulting HTML file.

@gyurielf
Copy link
Contributor

gyurielf commented Nov 20, 2023

For adapter-static, the trailingSlash = 'always' comes onto the table. You can see the actual build output: stackblitz.com/edit/sveltejs-kit-template-default-mt72gz
If you downgrade to 1.27.3 and build it, you can compare the .html entry point outputs.

The redirects are technically the correct outcome here. Since the trailingSlash is set to 'always', you should be redirecting to URLs with a trailing slash to avoid another redirect. I've done so in the repro below. https://stackblitz.com/edit/sveltejs-kit-template-default-q9l2pa?file=build%2Fsignin%2Findex.html

Incidentally, the behaviour that your app is trying to produce (the same SPA setup for every HTML file) is exactly what was meant to be fixed. When prerender is set to true or 'auto', the result should always be the same as navigating to the page in the browser and saving the resulting HTML file.

For us the trailingSlash needed for generating every route entries (its own folder and index.html) for the CDN.

Therefore the problem is, we are lost the ability to evaluate (eg a cookie) on let's say /profile page and stay on that page if the user is authenticated. Now we redirected every single time to the auth page if we hit the rout directly. (and the auth page is redirect us to the root page (not to the /profile). No matter what the load fn contains, because if the user hits the route directly, it won't be loaded.

@eltigerchino
Copy link
Member

eltigerchino commented Nov 20, 2023

Thanks a ton for taking the time to setup the repo @eltigerchino, really appreciate it (and you managed to do a better job of simplifying the setup than I could)!

I had a look and it seems like the build process does not prerender anything (.sveltekit/output has no prerendered folder after npm run build).

By replacing +server.ts with +page.ts with the only content being export const prerender = true;, I am now getting the build to attempt to prerender the index.html file (and failing with the error mentioned initially + the prerendered index.html is referencing the /signin route).

Link to fork

The +page.js file and the prerender = true option overrides the +layout.js with prerender = false. This means that the load function will run and redirect to the /signin page. It's working as expected.

In the example below, I'm specifying the / route as a prerender entry in svelte.config.js. This does force the root +server.js file to be prerendered. In doing so, I discovered a bug where the root server file always prerenders a html file even when the content-type is not html.
https://stackblitz.com/edit/sveltejs-kit-template-default-s3ayl2?file=src%2Froutes%2F(app)%2F%2Bserver.js,svelte.config.js

@eltigerchino
Copy link
Member

eltigerchino commented Nov 20, 2023

we are lost the ability to evaluate (eg a cookie) on let's say /profile page and stay on that page if the user is authenticated. Now we redirected every single time to the auth page if we hit the rout directly. (and the auth page is redirect us to the root page (not to the /profile). No matter what the load fn contains, because if the user hits the route directly, it won't be loaded.

This is the expected result of prerendering your pages. In your case, setting it to 'auto' has the exact same effect as true. The previous behaviour that you relied on was a bug (SPA page instead of actually capturing the fully rendered page when prerendering). It will need to be decided if we want to re-implement the behaviour that you were relying on in a different way (e.g., a config option for the static adapter). Right now, it's recommended that you set prerender = false and setup your static host to redirect all requests to 200.html (the fallback page). In most cases, this is usually the default for static hosts and would not require any configuration.
https://kit.svelte.dev/docs/single-page-apps

@gyurielf
Copy link
Contributor

we are lost the ability to evaluate (eg a cookie) on let's say /profile page and stay on that page if the user is authenticated. Now we redirected every single time to the auth page if we hit the rout directly. (and the auth page is redirect us to the root page (not to the /profile). No matter what the load fn contains, because if the user hits the route directly, it won't be loaded.

This is the expected result of prerendering your pages. In your case, setting it to 'auto' has the exact same effect as true. The previous behaviour that you relied on was a bug (SPA page instead of actually capturing the fully rendered page when prerendering). It will need to be decided if we want to re-implement the behaviour that you were relying on in a different way (e.g., a config option for the static adapter). Right now, it's recommended that you set prerender = false and setup your static host to redirect all requests to 200.html (the fallback page). In most cases, this is usually the default for static hosts and would not require any configuration. https://kit.svelte.dev/docs/single-page-apps

I see.

Also we have pages where we need to parse and use searchParams to load the page correctly.
How could we do that this way ? If e.g somebody wants share a /dashboard/transactions?from=1700411218&to=1700497596.
How cant we lost this searchParams if the user hits the /dashboard/transactions/index.html (which is actually just a raw redirect) ?

@eltigerchino
Copy link
Member

Also we have pages where we need to parse and use searchParams to load the page correctly. How could we do that this way ? If e.g somebody wants share a /dashboard/transactions?from=1700411218&to=1700497596. How cant we lost this searchParams if the user hits the /dashboard/transactions/index.html (which is actually just a raw redirect) ?

If you disable prerendering, it won't be a raw redirect. All requests should hit the 200.html page and that will run your load functions at runtime.

@austerj
Copy link
Author

austerj commented Nov 20, 2023

Thanks a ton for taking the time to setup the repo @eltigerchino, really appreciate it (and you managed to do a better job of simplifying the setup than I could)!
I had a look and it seems like the build process does not prerender anything (.sveltekit/output has no prerendered folder after npm run build).
By replacing +server.ts with +page.ts with the only content being export const prerender = true;, I am now getting the build to attempt to prerender the index.html file (and failing with the error mentioned initially + the prerendered index.html is referencing the /signin route).
Link to fork

The +page.js file and the prerender = true option overrides the +layout.js with prerender = false. This means that the load function will run and redirect to the /signin page. It's working as expected.

In the example below, I'm specifying the / route as a prerender entry in svelte.config.js. This does force the root +server.js file to be prerendered. In doing so, I discovered a bug where the root server file always prerenders a html file even when the content-type is not html. https://stackblitz.com/edit/sveltejs-kit-template-default-s3ayl2?file=src%2Froutes%2F(app)%2F%2Bserver.js,svelte.config.js

This makes sense to me - and the initial setup did not (since I would expect some conflict resolution when +page and +layout settings diverge). Yet this was the only way I managed to get the desired behavior, and I don't see how to replicate this now?

In your new example, it does prerender the GET endpoint - but as {"data":"ok"}, not the app shell as desired. Remove the GET endpoint and the error returns - downgrade to 1.27.3 and rename +server.js to +page.js (with the conflicting prerender configuration) and the app shell is prerendered with the layout load function called at run-time.

What would be the "correct" way to replicate this behavior post-1.27.3?

@eltigerchino
Copy link
Member

This makes sense to me - and the initial setup did not (since I would expect some conflict resolution when +page and +layout settings diverge).

From the documentation: "Child layouts and pages override values set in parent layouts, so — for example — you can enable prerendering for your entire app then disable it for pages that need to be dynamically rendered."

https://kit.svelte.dev/docs/page-options

Yet this was the only way I managed to get the desired behavior, and I don't see how to replicate this now?
In your new example, it does prerender the GET endpoint - but as {"data":"ok"}, not the app shell as desired.

That's the expected response because it's what's returned from +server.js.

Remove the GET endpoint and the error returns
downgrade to 1.27.3 and rename +server.js to +page.js (with the conflicting prerender configuration) and the app shell is prerendered with the layout load function called at run-time.

The previous behaviour in 1.27.3 is a bug where the load function is not being run when prerendered. The load function should always run during prerendering.

What would be the "correct" way to replicate this behavior post-1.27.3?

The correct way to render an SPA app shell is to turn off prerendering and use the fallback option for adapter-static.
https://kit.svelte.dev/docs/single-page-apps#usage

@austerj
Copy link
Author

austerj commented Nov 20, 2023

The correct way to render an SPA app shell is to turn off prerendering and use the fallback option for adapter-static. https://kit.svelte.dev/docs/single-page-apps#usage

I cannot use adapter-static since my API / auth endpoints rely on the server and authentication routes should be outside the app shell.

@eltigerchino
Copy link
Member

eltigerchino commented Nov 21, 2023

I’m wondering if the old behaviour should be reimplemented as another prerender option such as prerender = ‘spa’ or if the fix should just be reverted entirely.

@eltigerchino eltigerchino added the needs-decision Not sure if we want to do this yet, also design work needed label Nov 21, 2023
@gyurielf
Copy link
Contributor

Also we have pages where we need to parse and use searchParams to load the page correctly. How could we do that this way ? If e.g somebody wants share a /dashboard/transactions?from=1700411218&to=1700497596. How cant we lost this searchParams if the user hits the /dashboard/transactions/index.html (which is actually just a raw redirect) ?

If you disable prerendering, it won't be a raw redirect. All requests should hit the 200.html page and that will run your load functions at runtime.

Let me check please. (Today evening)
Because I'm afraid that this way we will lost the query string. CDN can't handle this. But we'll see.
I'm coming back today.

@austerj
Copy link
Author

austerj commented Dec 19, 2023

I’m wondering if the old behaviour should be reimplemented as another prerender option such as prerender = ‘spa’ or if the fix should just be reverted entirely.

Just wondering if the issue is still on the radar or if there's anything I can do from my side to help? Unfortunately not that familiar with SvelteKit internals, but I'm a bit worried about potentially being stuck on 1.27.3 indefintely, since I haven't managed to find another way to prerender the index.html shell while keeping the authentication fetches at run-time and by now (8 months into development) switching frameworks would be a huge pain.

@gyurielf
Copy link
Contributor

Also we have pages where we need to parse and use searchParams to load the page correctly. How could we do that this way ? If e.g somebody wants share a /dashboard/transactions?from=1700411218&to=1700497596. How cant we lost this searchParams if the user hits the /dashboard/transactions/index.html (which is actually just a raw redirect) ?

If you disable prerendering, it won't be a raw redirect. All requests should hit the 200.html page and that will run your load functions at runtime.

Let me check please. (Today evening) Because I'm afraid that this way we will lost the query string. CDN can't handle this. But we'll see. I'm coming back today.

Sorry for the late answer.. but things happened.. work, life etc..
I made several test and query strings not working with this approach.

@eltigerchino eltigerchino added this to the soon milestone Dec 19, 2023
@eltigerchino eltigerchino added bug Something isn't working and removed needs-decision Not sure if we want to do this yet, also design work needed labels Dec 19, 2023
@eltigerchino
Copy link
Member

@austerj @gyurielf I've submitted a fix that restores the previous behaviour prerendering a shell page when SSR is turned off and there is no server load function for that page. This avoids running load functions during prerendering if there are no server loads we need to prerender the __data.json file for. Let me know what you guys think!

@austerj
Copy link
Author

austerj commented Dec 20, 2023

Just had a chance to test out your fix @eltigerchino and can confirm that it solves the issue and my project is building again on the new version, thank you so much! Also really appreciate adding the test so I can sleep at night without worrying about this in the future.

@gyurielf In case you want to check if the fix works for you too, I had some issues in getting kit from the specific commit, but the steps that got it working for me in the end were:

  1. Update to the latest version of SvelteKit (e.g. replace "@sveltejs/kit": "1.27.3" with "@sveltejs/kit": "^1.30.3" in case you locked the version like I did)
  2. Check that npm run build reproduces your issue
  3. Manually replace node_modules/@sveltejs/kit/src with the one from kit-fix-no-ssr-regression/packages/kit/src
  4. Check that npm run build now gets your desired behavior

I also had to fix my import of vitePreprocess from @sveltejs/kit/vite to @sveltejs/vite-plugin-svelte - maybe something that changed in between the versions while I was staying on 1.27.3.

@gyurielf
Copy link
Contributor

Just had a chance to test out your fix @eltigerchino and can confirm that it solves the issue and my project is building again on the new version, thank you so much! Also really appreciate adding the test so I can sleep at night without worrying about this in the future.

@gyurielf In case you want to check if the fix works for you too, I had some issues in getting kit from the specific commit, but the steps that got it working for me in the end were:

  1. Update to the latest version of SvelteKit (e.g. replace "@sveltejs/kit": "1.27.3" with "@sveltejs/kit": "^1.30.3" in case you locked the version like I did)
  2. Check that npm run build reproduces your issue
  3. Manually replace node_modules/@sveltejs/kit/src with the one from kit-fix-no-ssr-regression/packages/kit/src
  4. Check that npm run build now gets your desired behavior

I also had to fix my import of vitePreprocess from @sveltejs/kit/vite to @sveltejs/vite-plugin-svelte - maybe something that changed in between the versions while I was staying on 1.27.3.

I'll test it and get back.

@eltigerchino
Copy link
Member

this is now released https://github.com/sveltejs/kit/releases/tag/%40sveltejs%2Fkit%402.0.5

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
3 participants