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

@sveltejs/adapter-cloudflare v2.2.0 always triggers hooks on build #9386

Closed
danawoodman opened this issue Mar 10, 2023 · 25 comments · Fixed by #11596
Closed

@sveltejs/adapter-cloudflare v2.2.0 always triggers hooks on build #9386

danawoodman opened this issue Mar 10, 2023 · 25 comments · Fixed by #11596

Comments

@danawoodman
Copy link
Contributor

danawoodman commented Mar 10, 2023

Describe the bug

Took a while to track down the cause of this, but I recently updated to adapter-cloudflare 2.2.0 and my builds started to break.

It appears a change from 2.1.0 to 2.2.0 now always triggers hooks.server.js to run, even if there are no configured prerenderable routes, which then triggers my auth middleware which redirects the user to another domain if they have no authentication cookie set (see hooks.server.js in the repro repo).

v2.1.0 works fine and I can build without the hook getting triggered by the adapter, but now in 2.2.0 it always triggers and thus breaks the build. AFAIK this means that there is no way to build for CloudFlare anymore if you have such an auth hook in place.

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-g2rzrj?file=src/hooks.server.js

Run npm run build to see the issue. Downgrade to 2.1.0 to see it go away

Logs

> Using @sveltejs/adapter-cloudflare
file:///Users/my-app/node_modules/@sveltejs/kit/src/core/postbuild/fallback.js:53
        throw new Error(`Could not create a fallback page — failed with status ${response.status}`);
              ^

Error: Could not create a fallback page — failed with status 302
    at generate_fallback (file:///Users/my-app/node_modules/@sveltejs/kit/src/core/postbuild/fallback.js:53:8)
    at async process.<anonymous> (file:///Users/my-app/node_modules/@sveltejs/kit/src/utils/fork.js:25:17)
error during build:
Error: Failed with code 1
    at ChildProcess.<anonymous> (file:///Users/my-app/node_modules/@sveltejs/kit/src/utils/fork.js:68:13)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:293:12)

System Info

System:
    OS: macOS 13.2.1
    CPU: (8) arm64 Apple M2
    Memory: 740.67 MB / 24.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.19.0 - ~/Library/Caches/fnm_multishells/80853_1678404247439/bin/node
    npm: 8.19.3 - ~/Library/Caches/fnm_multishells/80853_1678404247439/bin/npm
  Browsers:
    Brave Browser: 106.1.44.112
    Chrome: 111.0.5563.64
    Firefox: 103.0
    Safari: 16.3
  npmPackages:
    @sveltejs/adapter-auto: 2.0.0 => 2.0.0 
    @sveltejs/adapter-cloudflare: 2.1.0 => 2.1.0 
    @sveltejs/kit: 1.11.0 => 1.11.0 
    svelte: 3.55.1 => 3.55.1 
    vite: 4.1.4 => 4.1.4

Severity

breaks production builds, no work around except downgrading

Additional Information

No response

@Conduitry
Copy link
Member

I'm guessing this is the result of #9294.

I'm not sure what it would make sense to do about this. Can you check building from $app/environment and not apply any auth middleware in that case?

@stefandevo
Copy link

Same issue here. I am now checking with the building value, but this is a weird work around...

@danawoodman
Copy link
Contributor Author

@Conduitry yeah, I had a feeling it was from #9294 as well based on it being the main change from 2.1.0 to 2.2.0.

I could probably do that, but as @stefandevo says, it is a weird workaround/hack. I would assume that Kit would not run my hooks if I have no pre-rendering configured anywhere. For now I'll stay on 2.1.0 but if we decide to update I'll do the suggestion you offer, thanks 👍

@paulgibbs
Copy link

I have also encountered this issue, with auth middleware as well.

@bobby
Copy link

bobby commented Mar 29, 2023

Same issue here.

@ajgeiss0702
Copy link
Contributor

ajgeiss0702 commented Mar 31, 2023

Can you check building from $app/environment and not apply any auth middleware in that case?

This seems like the best solution for now, unless an option is added to disable generation of a 404.html from the cloudflare adapter (which rich pushed back on before when I tried to add it so i'm not sure if that would happen. Although then neither of us could think of a case where it would make sense to be disabled, I wanted it "just in case" something like this happened)


I would say the solution on the svelte side could be:

  • Add an option to disable 404.html generation

and/or

  • Document the generation of 404.html on the docs

@eltigerchino
Copy link
Member

eltigerchino commented Apr 1, 2023

Instead of always generating a fallback page, I suggest that the cloudflare adapter copies over a static 404.html to the /_app/ or / root directory. The <head> of this file should include <meta http-equiv="Refresh" content="0; url='/'" /> to redirect to the root page only if visited through a browser.

The benefits of this approach are:

  • avoids running hooks during build.
  • returns 404 when requesting missing assets in /_app/* or routes excluded by a wildcard.

    the original issue was that it would return /index.html and a 200 status instead of 404.

  • redirects users to / when navigating to these non-existent excluded routes.

    excluded routes in _routes.json bypass the Cloudflare worker and returns the page html file or nearest index.html / 404.html.

Although this solution still doesn't really sit right with me, I feel that it is a good compromise.

@ajgeiss0702
Copy link
Contributor

ajgeiss0702 commented Apr 1, 2023

Instead of always generating a fallback page, I suggest that the cloudflare adapter copies over a static 404.html to the /_app/ or / root directory. The <head> of this file should include <meta http-equiv="Refresh" content="0; url='/'" /> to redirect to the root page only if visited through a browser.

The benefits of this approach are:

* avoids running hooks during build.

* returns 404 when requesting missing assets in `/_app/*` or routes excluded by a wildcard.
  > the original issue was that it would return `/index.html` and a 200 status instead of 404.

* redirects users to `/` when navigating to these non-existent excluded routes.
  > excluded routes in `_routes.json` bypass the Cloudflare worker and returns the page html file or nearest `index.html` / `404.html`.

Although this solution still doesn't really sit right with me, I feel that it is a good compromise.

Just redirecting to the root doesn't seem like a good solution at all to me. Instead of getting a 404 page, users would just get redirected to the root page, which almost defeats the purpose of a 404 page at all.

I feel the standard that works for most sites, and matches the functionality of other adapters is better than having a different behavior that doesn't make a ton of sense for most sites. It should either be an option, or just something that should be worked around (as the workaround is pretty simple. It should probably still be documented though)

@eltigerchino
Copy link
Member

eltigerchino commented Apr 2, 2023

I feel the standard that works for most sites, and matches the functionality of other adapters is better than having a different behavior that doesn't make a ton of sense for most sites.

Perhaps the fallback page should also be opt-in and print a warning if it's not configured?
Rather than having it automatically generated and confusing users why their build idea failing.

Or perhaps we should document and warn users about their hooks whenever the fallback page generator fails (this would benefit the static adapter as well).

In the end, it's difficult to get a behaviour consistent with other adapters until cloudflare removes the 100 rule limit of the _routes.json file.

@dbradleyfl
Copy link

I've had to revisit this issue a couple different times since this hasn't been fixed and I keep forgetting why my sveltekit apps won't build in Pages but will locally 😞 The answer isn't explicitly here in code yet, so sharing what worked for me based on the above suggestions:

src/hooks.server.ts

// other imports here
import { building } from '$app/environment'

export async function handle({ event, resolve }) {
  if (building) {
    const response = await resolve(event)
    return response // bailing here allows the 404 page to build
  }

// rest of the complex server code down here that was causing 404 page not to build

@figuerom16
Copy link

Is there a temporary work around for this for Cloudflare Pages? I've tried downgrading to @sveltejs/[email protected] and using @dbradleyfl method if (building) await resolve(event), but I get the same "Error: Could not create a fallback page — failed with status 303" when deploying to Cloudflare Pages.

@ajgeiss0702
Copy link
Contributor

Is there a temporary work around for this for Cloudflare Pages? I've tried downgrading to @sveltejs/[email protected] and using @dbradleyfl method if (building) await resolve(event), but I get the same "Error: Could not create a fallback page — failed with status 303" when deploying to Cloudflare Pages.

It sounds like your site is still processing the hooks during build. Make sure that if building stops execution if it is building

@dbradleyfl
Copy link

Yup sounds like you're not returning the result of await resolve(event). The return is the important part @figuerom16

@figuerom16
Copy link

figuerom16 commented May 13, 2023

@ajgeiss0702 @dbradleyfl Doh! You're correct. I forgot the return. Works with the newest @sveltejs/[email protected]. Thank you much!
if (building) return await resolve(event)

@RowanAldean
Copy link

I've had to revisit this issue a couple different times since this hasn't been fixed and I keep forgetting why my sveltekit apps won't build in Pages but will locally 😞 The answer isn't explicitly here in code yet, so sharing what worked for me based on the above suggestions:

src/hooks.server.ts

// other imports here
import { building } from '$app/environment'

export async function handle({ event, resolve }) {
  if (building) {
    const response = await resolve(event)
    return response // bailing here allows the 404 page to build
  }

// rest of the complex server code down here that was causing 404 page not to build

You're the GOAT, for anyone having this specific problem - this is the issue. Cloudflare Pages build failing due to auth redirect on base path.

Figured i'd write it so this ranks on Google! Saved me time by putting me on to building brother - thank you!

@socketopp
Copy link

socketopp commented Jan 3, 2024

  if (building) {
    const response = await resolve(event)
    return response // bailing here allows the 404 page to build
  }

How do you add this if you're using sequence along with SvelteKitAuth?
Doesn't seem to fix my error by adding resolve(event) in the handle that handles my routes in my sequence.

Update
#7899
Found a solution to conditionally render sequence depending if we're building or not.

@givenergy-dexter
Copy link

  if (building) {
    const response = await resolve(event)
    return response // bailing here allows the 404 page to build
  }

How do you add this if you're using sequence along with SvelteKitAuth? Doesn't seem to fix my error by adding resolve(event) in the handle that handles my routes in my sequence.

Update #7899 Found a solution to conditionally render sequence depending if we're building or not.

Do you have your source code for this?

@socketopp
Copy link

  if (building) {
    const response = await resolve(event)
    return response // bailing here allows the 404 page to build
  }

How do you add this if you're using sequence along with SvelteKitAuth? Doesn't seem to fix my error by adding resolve(event) in the handle that handles my routes in my sequence.
Update #7899 Found a solution to conditionally render sequence depending if we're building or not.

Do you have your source code for this?

export const handle = building
    ? sequence()
    : sequence(...);

@givenergy-dexter
Copy link

  if (building) {
    const response = await resolve(event)
    return response // bailing here allows the 404 page to build
  }

How do you add this if you're using sequence along with SvelteKitAuth? Doesn't seem to fix my error by adding resolve(event) in the handle that handles my routes in my sequence.
Update #7899 Found a solution to conditionally render sequence depending if we're building or not.

Do you have your source code for this?

export const handle = building
    ? sequence()
    : sequence(...);

Yeah, nice one, I ended up doing:

import { sequence } from "@sveltejs/kit/hooks";

import { building } from '$app/environment'

function optional (fn) {
  if (building) {
    return ({ event, resolve }) => resolve(event)
  }

  return fn
}

export const handle = optional(sequence())

@socketopp
Copy link

  if (building) {
    const response = await resolve(event)
    return response // bailing here allows the 404 page to build
  }

How do you add this if you're using sequence along with SvelteKitAuth? Doesn't seem to fix my error by adding resolve(event) in the handle that handles my routes in my sequence.
Update #7899 Found a solution to conditionally render sequence depending if we're building or not.

Do you have your source code for this?

export const handle = building
    ? sequence()
    : sequence(...);

Yeah, nice one, I ended up doing:

import { sequence } from "@sveltejs/kit/hooks";

import { building } from '$app/environment'

function optional (fn) {
  if (building) {
    return ({ event, resolve }) => resolve(event)
  }

  return fn
}

export const handle = optional(sequence())

Do you see any benefits from doing one to another?

@givenergy-dexter
Copy link

Do you see any benefits from doing one to another?

No, just a bit more readable I suppose.

@Rich-Harris
Copy link
Member

Can someone sanity check #11596 and tell me if it seems like a good fix for this?

@SaintPepsi
Copy link

I wanted to add to this, I got the same issue, even though I was using adapter-static

I was getting an error relating to @inlang/paraglide-js since you add a handler in the hooks.server.ts that I presume redirects the user: i18n.handle()

I had to wrap adding this to my Handle sequence like so:

const sequences: Handle[] = [];

if (!building) {
	sequences.push(i18n.handle({ disableAsyncLocalStorage: true }));
}

sequences.push(customHandle);

export const handle: Handle = sequence(...sequences);

And now it works. what I don't understand is why wrangler is giving me issues when I'm using a different adapter

I've got this function that helps me determine the correct adapter during runtime:

function getAdapter() {
	console.log('process.env.VITE_ADAPTER', process.env.VITE_ADAPTER);
	switch (process.env.VITE_ADAPTER) {
		case 'capacitor':
			return staticAdapter({
				strict: false,
				pages: 'build/capacitor',
				assets: 'build/capacitor',
				fallback: 'index.html',
			});

		case 'cloudflare-workers':
			return cloudflareWorkersAdapter();

		default:
			return adapterAuto();
	}
}

if I comment out the cloudflare-workers case (and subsequently no longer import cloudflareWorkersAdapter, I get a different error:

node:internal/event_target:1096
  process.nextTick(() => { throw err; });
                           ^
Error: Could not create a fallback page — failed with status 302

This goes way above my head lol but i'm happy to help if there's any directions

@eltigerchino
Copy link
Member

I wanted to add to this, I got the same issue, even though I was using adapter-static

I was getting an error relating to @inlang/paraglide-js since you add a handler in the hooks.server.ts that I presume redirects the user: i18n.handle()

I had to wrap adding this to my Handle sequence like so:

const sequences: Handle[] = [];

if (!building) {
	sequences.push(i18n.handle({ disableAsyncLocalStorage: true }));
}

sequences.push(customHandle);

export const handle: Handle = sequence(...sequences);

And now it works. what I don't understand is why wrangler is giving me issues when I'm using a different adapter

I've got this function that helps me determine the correct adapter during runtime:

function getAdapter() {
	console.log('process.env.VITE_ADAPTER', process.env.VITE_ADAPTER);
	switch (process.env.VITE_ADAPTER) {
		case 'capacitor':
			return staticAdapter({
				strict: false,
				pages: 'build/capacitor',
				assets: 'build/capacitor',
				fallback: 'index.html',
			});

		case 'cloudflare-workers':
			return cloudflareWorkersAdapter();

		default:
			return adapterAuto();
	}
}

if I comment out the cloudflare-workers case (and subsequently no longer import cloudflareWorkersAdapter, I get a different error:

node:internal/event_target:1096
  process.nextTick(() => { throw err; });
                           ^
Error: Could not create a fallback page — failed with status 302

This goes way above my head lol but i'm happy to help if there's any directions

I recommend creating a new discussion (instead of an issue) with a minimal reproduction. It's difficult to diagnose the root cause of your problem from your current description.

@SaintPepsi
Copy link

Thanks @eltigerchino I'm not sure what the discussion would be about but I've created a minimal reproducable: https://github.com/SaintPepsi/paraglide-sveltekit-example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment