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

feat: update to latest Kit adapter API #153

Closed
jthegedus opened this issue Nov 2, 2021 · 19 comments · Fixed by #159
Closed

feat: update to latest Kit adapter API #153

jthegedus opened this issue Nov 2, 2021 · 19 comments · Fixed by #159
Assignees
Labels
enhancement New feature or request

Comments

@jthegedus
Copy link
Owner

jthegedus commented Nov 2, 2021

Describe the problem

Kit Adapter API has changed significantly over the Dec/Jan holiday period, it needs to be updated.

Describe the proposed solution

Alternatives considered

NA

Importance

i cannot use svelte-adapter-firebase without it


I will tackle this by end of the month (Jan2022) as I have been on and have further upcoming leave (computer absence) so cannot commit to a resolution or reviews sooner than this.

@jthegedus jthegedus added the enhancement New feature or request label Nov 2, 2021
@jthegedus jthegedus self-assigned this Nov 2, 2021
@jrmoynihan
Copy link

jrmoynihan commented Jan 7, 2022

Definitely would want to include the SSR route-splitting and other adapter API changes in here. Rich has been busy since joining Vercel!
sveltejs/kit#2931
sveltejs/kit#3126
sveltejs/kit#3133

@jthegedus
Copy link
Owner Author

Also want to mimic the immutability for static assets from sveltejs/kit#3222 in this update too.

@vpanyushenko
Copy link

What would be the main benefit of route spitting with google's cloud functions? I understand that in theory if you had one route that was resource heavy you would be able to run it with a function that has more resources using the runWith method.

@jthegedus You mentioned in your comment that GCP is more performant without route splitting. Is there a reason why? I would assume it wouldn't have much performance impact unless perhaps matching routes to function rewrites in the firebase.json has some overhead.

@jthegedus
Copy link
Owner Author

I am not completely across sveltejs/kit#2931 so may be wrong, but as I understand it, route-splitting in this context is so we could have many Cloud Functions pointing to different pages of our app. Assuming this is what is meant by route splitting, Kit Adapter API would support:

  1. 1:* CF to all app routes
  2. *:* CFs to app routes or groups of routes
  3. 1:1 CF per app route

1 is what currently happens. 3 is what everyone wants because of the popularization of "edge function" platforms. 2 might appear to be a more manageable version of 3, but I would wager not worth the effort for 99% of use cases.

So from this point discussion will be kept to points 1 and 3.

What would be the main benefit of route spitting with google's cloud functions? I understand that in theory if you had one route that was resource heavy you would be able to run it with a function that has more resources using the runWith method.

The benefit of route splitting as stated in sveltejs/kit#2931 is to:

  • reduce size of bundled JS to support more space-restrictive runtime environments (CloudFlare Workers 1MB, AWS Lambda 50MB / 250MB, etc)

with secondary improvements being:

  • faster JS runtime boot due to less JS code to parse for a particular route (only parsing code for that route as opposed to all code for all routes)
  • faster route lookup in SvelteKit

So the main reason for route splitting does not improve usability of Cloud Functions like it does other environments (Firebase Cloud Function quota shows 100MB / 500MB deployment size limit).

With the current Firebase Adapter setup you can only set runWith for the single function that is produced, which means all your code will run with whatever settings you configure. Since runWith is used to INCREASE the specs of the machine, I can't think of a situation where anyone would want the other parts of their code to run on a slower machine. I haven't done any quantitative analysis on the perf/cost ratios of the different Cloud Function resource config, but typically use the 512MB/1GB 1vCPU config as I have observed more than 2x faster startup & processing for Node.js runtimes than with the default 256MB config.

The secondary improvements are nice-to-haves IMO, but introduce other challenges with this adapters usability (more later).

@jthegedus You mentioned in your comment that GCP is more performant without route splitting. Is there a reason why? I would assume it wouldn't have much performance impact unless perhaps matching routes to function rewrites in the firebase.json has some overhead.

There is a performance impact in that each route is a different Cloud Function and therefore each route incurs the cold-start penalty of Cloud Functions instead of the first route being the only cold-start. It is strictly true that there would be more cold-starts. Having said that, the impact on users depends on the application and user navigation patterns.

For example, take a user that lands on the main app page and makes 10 subsequent in-app routes, with 8 being new pages and 2 being repeat page visits. With the two options we have:

  • 1:1: 1 initial page visit + 8 other page visits = 9 routes = 9 Cloud Functions = 9 cold-starts
  • 1:*: 1 initial page visit + 8 other page visits = 9 routes = 1 Cloud Functions = 1 cold-starts

Needlessly opting-in to more cold-starts seems like a bad idea to me, and I'd guess for 99% of apps that the secondary benefits of faster Kit route lookup and faster JS parsing would not be a valuable tradeoff for the cold-start boot times.

In addition to these considerations, the Firebase deploy API only supports 80 Cloud Function deployments per 100 seconds which may be hit should your many functions change frequently enough.


Now, the actual aspect of all this I have issue with is the usability of this adapter. The current design requires a firebase.json and looks up the "catch-all" rewrite rule to identify the Function name defined by the user ahead of time so the Cloud Function code can be generated and manually added to the functions dir code. The motivation here is that Firebase projects can support multiple apps and the Cloud Function code is managed in a project-specific way, so I didn't want to be prescriptive. Effectively, the firebase.json file is an input into the adapter to cater for multiple apps and user-specific Cloud Function code organisation.

The Kit adapter API seems to be moving in a direction that treats the target platform config as an output, rather than an input, which is fine for the other platforms as they only support 1 app per "project/account" whereas Firebase is a many app per "project/account" platform. Modifying the Firebase project's firebase.json file during the build-phase for one app in the project is a usability challenge. It's certainly doable, but will take me some time to figure out the ergonomics I am happy with.

@evdama
Copy link

evdama commented Jan 15, 2022

The Kit adapter API seems to be moving in a direction that treats the target platform config as an output, rather than an input, which is fine for the other platforms as they only support 1 app per "project/account" whereas Firebase is a many app per "project/account" platform.

Thank you very much for your amazing work on this adapter 😃 👏
I'm giving you a users perspective on this...
So I've done an app using Sapper and a collection of firebase services (auth, firestore, functions, hosting). So far so good... worked well but was an insane effort to configure everything including the deployment (all in all over one year of constant work until all was finished).

Then along came the announcement that Sapper would be discontinued and SveleKit was going to be its successor, also, of course, at around the same time the firebase SDK made the move from v8 of their API to v9. All in all, a migration of said app became practically impossible to do... in theory all seems easy and just a little bit of effort... in practice it never is of course...

So here we are, I'd like to start over, build a new app using SvelteKit this time. I also need some cloud based services... auth, serverless functions, some cloud database and of course hosting... I'm looking around for 2 months now... there's simply at this point no stack that in my opinion just works the way Sapper and firebase v8 did... is there? So what shall we do? Best thing I found is this adapter but it also seems to hit a roadblock now... no? Honest question, what would you do if you want to build an app using auth, serverless functions, hosting, some database... I've gone full-cicle... looked at vercel, netifly... same everywhere... only one or two bits 'might' work, no docs, no tutorials on either stack or platform... hm... what to do? 🤷

Back on point: what if you don't worry to much about the one platform many apps approach at this point, what if you just make it work for one (as this seems to cater 90% of users and use cases) and maybe at a later point try to cover for the use case of one account / many apps? This way we would get at least one stack working in this damn jungle of confusion and half half things nobody has really figured out yet it seems 😆

@vpanyushenko
Copy link

there's simply at this point no stack that in my opinion just works the way Sapper and firebase v8 did

Svelte kit and Firebase work great together. V8 and V9 both work, V9 just just a smaller bundle size.

Regarding support for multiple sites, (one platform, many apps) the issue isn't really with an output for multiple sites. For example, you can have two or three directories, each with a unique svelte kit site, and the adapter works perfectly for it.

The difficulties are with the route splitting on a single site that svelte kit now offers, because a single site would have multiple cloud functions associated with it (one function per route). That can cause some headaches because the adapter would be writing many functions to to your project's index.js file and the deployment would potentially hit the firebase functions deploy limit.

Because firebase looks for the function to run in firebase.json, for each function that gets added the firebase.json would need to be updated and build time. There are issues with modifying the user's input config file automatically, and @jthegedus is yet to find a solution that he likes.

In my option, the most simple solution would be to use the: 1:* CF to all app routes that the current adapter uses. That can solve issues for the majority of developers using this adapter besides those who have a bundle size that is too large, which for Firebase developers I don't think is very common.

@pozsa
Copy link

pozsa commented Jan 31, 2022

@jthegedus you mentioned that you intend to make the updates by the of Jan 2022, which is today I guess. :)
When do you think a new version can be expected? Thanks.

@gustavopch
Copy link

gustavopch commented Jan 31, 2022

@pozsa While this adapter is not updated, you can implement your own locally by copying the code from this repo and adjusting to the breaking changes by following SvelteKit's changelog and also using their official adapters for reference.

I've done it already, so I'm pasting the result here, but I haven't tested it after the latest breaking changes, so it's probably broken. Also, I've removed all the configuration boilerplate and hard-coded everything, so you'll need to change some stuff to fit your own project.

adapter/index.js
import * as ESBuild from 'esbuild'
import * as FS from 'fs'
import * as Path from 'path'
import * as URL from 'url'

const HOSTING_TARGET = 'web'

const adapter = function () {
  return {
    name: 'svelte-adapter-firebase',
    async adapt(builder) {
      const { filesDir, serverDir, publicDir, tmpDir, nodeVersion } = (() => {
        const firebaseJson = JSON.parse(
          FS.readFileSync(Path.resolve('./firebase.json'), 'utf8'),
        )

        const hostingConfig = firebaseJson.hosting.find(
          ({ target }) => target === HOSTING_TARGET,
        )

        const rewriteConfig = hostingConfig.rewrites.find(
          ({ source }) => source === '**',
        )

        const functionsDir = Path.resolve(firebaseJson.functions.source)
        const functionsPkgJson = JSON.parse(
          FS.readFileSync(Path.join(functionsDir, 'package.json'), 'utf8'),
        )

        const thisDir = Path.dirname(URL.fileURLToPath(import.meta.url))

        return {
          filesDir: Path.join(thisDir, './files'),
          serverDir: Path.join(functionsDir, Path.dirname(functionsPkgJson.main), rewriteConfig.function), // prettier-ignore
          publicDir: Path.resolve(hostingConfig.public),
          tmpDir: builder.getBuildDirectory('firebase'),
          nodeVersion: firebaseJson.functions.runtime.replace('nodejs', ''),
        }
      })()

      builder.rimraf(tmpDir)
      builder.rimraf(serverDir)

      builder.log.minor('Prerendering static pages')

      await builder.prerender({
        dest: publicDir,
      })

      builder.log.minor('Generating Cloud Function')

      const relativePath = Path.posix.relative(
        tmpDir,
        builder.getServerDirectory(),
      )

      builder.copy(filesDir, tmpDir, {
        replace: {
          APP: `${relativePath}/app.js`,
          MANIFEST: './manifest.js',
        },
      })

      FS.writeFileSync(
        `${tmpDir}/manifest.js`,
        `export const manifest = ${builder.generateManifest({
          relativePath,
        })};\n`,
      )

      await ESBuild.build({
        bundle: true,
        entryPoints: [Path.join(tmpDir, 'entry.js')],
        outfile: Path.join(serverDir, 'index.js'),
        platform: 'node',
        target: `node${nodeVersion}`,
      })

      builder.log.minor('Copying assets')

      builder.writeStatic(publicDir)
      builder.writeClient(publicDir)
    },
  }
}

export default adapter
adapter/files/entry.js
import { __fetch_polyfill } from '@sveltejs/kit/install-fetch'
import { App } from 'APP'
import { manifest } from 'MANIFEST'
import { Readable } from 'stream'

__fetch_polyfill()

const app = new App(manifest)

const entry = async (req, res) => {
  const base = `http://${req.headers['host']}`

  const rendered = await app.render(
    new Request(base + req.url, {
      method: req.method,
      headers: Object.entries(req.headers).reduce((acc, [key, value]) => {
        acc[key] = Array.isArray(value) ? value.join(',') : value
        return acc
      }, {}),
      rawBody: req.rawBody ?? null,
    }),
  )

  res.writeHead(rendered.status, Object.fromEntries(rendered.headers))

  if (rendered.body instanceof Readable) {
    rendered.body.pipe(res)
  } else if (rendered.body) {
    res.write(await rendered.arrayBuffer()).end()
  } else {
    res.end()
  }
}

export default entry

@reinoute
Copy link

@gustavopch do you have an example repo to set this up? I tried and failed.

Ofcourse, a new release would be even better :)

@gustavopch
Copy link

@reinoute Unfortunately, not. It's just a starting point. I haven't deployed to Firebase since the last breaking changes, so that code is probably actually broken.

@reinoute
Copy link

@jthegedus are there still plans to deliver this? Otherwise I will have to go for the App Engine adapter, which I'd rather not use.

In any case, thanks a lot for all your work on this library.

@gustavopch
Copy link

@reinoute I've updated the adapter/files/entry.js file in my comment above. It works here. You'll still need to adjust adapter/index.js according to how your project is set up though.

@reinoute
Copy link

How do I set these paths @gustavopch ? Do you have an example?

import { App } from 'APP'
import { manifest } from 'MANIFEST'

@gustavopch
Copy link

gustavopch commented Feb 17, 2022

@reinoute You don't. Those paths are replaced when the adapter copies that file to the build directory:

builder.copy(filesDir, tmpDir, {
  replace: {
    APP: `${relativePath}/app.js`,
    MANIFEST: './manifest.js',
  },
})

@jthegedus
Copy link
Owner Author

jthegedus commented Feb 22, 2022

Hi all, apologies for the delay. Dec-Jan I was on leave and AFK the entire time, Jan-Feb I have been slammed with work, I have had absolutely zero time to work on any open source software, sad times. I am back at it though and will be going through the Kit updates, the feedback shared in this thread, and the examples shared to deliver a working solution sometime in the next 7 days.

I appreciate your patience, please bear with me a little longer.

Just to add to the set of changes, Cloud Functions v2 were announced last week so I imagine Firebase will have an update on this too. So a lot of changes merge and get right.

@andreavaccari
Copy link

Hi @jthegedus, thank you for your work on this adapter. Would you be able to share an update on your progress? I'm certainly not trying to apply pressure, just need to plan our work accordingly Thank you again for your contributions!

@okydk
Copy link

okydk commented Mar 21, 2022

I also have a project which will rely on this adapter. Can we help with PRs or sponsoring the missing parts? @jthegedus

@nielsvandermolen
Copy link
Contributor

Could we do an adapter update without implementing the route splitting functions and new firebase cloud functions?

I am willing to help as well. As I understand the build process now it is first running the SvelteKit build. Then it moves the index.js output to the ssr folder in the functions directory. Are there some magic firebase steps that need to be implemented to the index.js file?

@andreavaccari
Copy link

Thank you @nielsvandermolen and @jthegedus! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants