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: route level config #8740

Merged
merged 46 commits into from
Feb 6, 2023
Merged

feat: route level config #8740

merged 46 commits into from
Feb 6, 2023

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Jan 26, 2023

closes #8383

Open questions:

  • builder.createEntries is probably the wrong place for this, because it only contains non-prerendered pages, but you could have prerendered pages which you want to deploy to different runtimes/regions (or am I mistaken here and this doesn't matter since assets always go to some special static cdn? even if, is the API general-purpose enough for this to be ok for all adapters?) - answer: yes createEntries is the correct API
  • multiple runtimes/regions means multiple functions. We need a to find a good way to find out which of those can live within one function, just splitting up every page/endpoint into its own function would not be ideal - answer: createManifest and filter enable this
  • as mentioned already in the issue, there's no good way currently to tell "these endpoints should all be deployed to the edge" without duplicating that config in each of them. Not sure how much this gets annoying in practice, maybe it's okay? - answer: with adapter-level defaults this shouldn't be an issue
  • which adapters besides Vercel benefit from this right now?

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

changeset-bot bot commented Jan 26, 2023

⚠️ No Changeset found

Latest commit: b3b1b6e

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

assets always go to some special static cdn?

I don't think there's ever a good reason for that not to happen — i think it's appropriate for prerendered routes to be excluded from this process. i'd almost go so far as to say that export const config and export const prerender = true in the same place should be an error, except that you might have both at the root for the sake of defaults then override them independently lower down

We need a to find a good way to find out which of those can live within one function

One idea is to use the same logic we currently use — if split: true, then we use the same splitting logic as presently, except for each group of routes with the same combination of runtime/region(s) configuration rather than for all routes. If split: false, we'd create a function for each group of routes with the same config.

Not sure if we need to account for memory and maxDuration options (for Vercel serverless functions) as well. Though I'd probably take the existence of those options as a signal that the route should be a standalone function (to the extent allowed by route ambiguity).

Another idea would be to make it explicit, with a property like group that can take an arbitrary group ID:

export const config = {
  runtime: 'edge',
  group: 'potato'
};

I don't love that idea though.

there's no good way currently to tell "these endpoints should all be deployed to the edge" without duplicating that config in each of them

I think it'd probably be enough to have a default config that applies to all routes in the absence of specified config. That would mean that you could (for example) have all your API routes run on Node while all your pages are rendered at the edge:

// svelte.config.js
export default {
  kit: {
    adapter: adapter({
      defaultConfig: {
        runtime: 'node18',
        region: 'us-east-1'
      }
    })
  }
};
// src/routes/+layout.server.js
export const config = {
  runtime: 'edge',
  region: 'all'
};

@dummdidumm
Copy link
Member Author

One idea is to use the same logic we currently use — if split: true, then we use the same splitting logic as presently, except for each group of routes with the same combination of runtime/region(s) configuration rather than for all routes. If split: false, we'd create a function for each group of routes with the same config.

I don't really understand the difference here to be honest. My take:

  • split: true is unchanged, you just split every route
  • configs are now taken into account to create one function per configuration set (all config parameters are taken into account to create groups)

packages/kit/types/index.d.ts Outdated Show resolved Hide resolved
packages/kit/types/private.d.ts Show resolved Hide resolved
packages/adapter-vercel/index.d.ts Outdated Show resolved Hide resolved
@Rich-Harris
Copy link
Member

@ascorbic wanted to flag this PR with you since it seems likely that adapter-netlify might also be able to take advantage of route-level deployment configuration

@dummdidumm
Copy link
Member Author

I bumped adapter-auto and create-svelte (tests will probably fail for the latter since the version in there are not released yet), so I believe this is good to go 🎉

@Rich-Harris Rich-Harris merged commit c7648f6 into master Feb 6, 2023
@Rich-Harris Rich-Harris deleted the route-level-config branch February 6, 2023 16:09
@github-actions github-actions bot mentioned this pull request Feb 6, 2023

## config

With the concept of [adapters](/docs/adapters), SvelteKit is able to run on a variety of platforms. Each of these might have specific configuration to further tweak the deployment — for example with Vercel or Netlify you could chose to deploy some parts of your app on the edge and others on serverless environments.
Copy link
Contributor

Choose a reason for hiding this comment

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

choose

@CanRau
Copy link

CanRau commented Feb 7, 2023

@dummdidumm very interesting and promising, could this in the future evolve into allowing to e.g. default deploy to Cloudflare Pages with some routes deploy externally to sthg like fly.io?

Just curious, actually kinda unsure how useful this is over having a monorepo auto-deploy using GitHub Actions the main app to CF and separate "functions" to fly or sthg like that.

It's probably more likely to stay in userland and/or on a per project basis I guess¿!
What do you think?

@dummdidumm
Copy link
Member Author

Right now config is intended to only work with one adapter at a time, so the use case you describe isn't possible today. I'm not sure how a "cross-adapter route level config" would look like / what implementation changes it would need. Anyway, if this is something you would like to see, feel free to open a feature request so we can at least track this (no promises about if/when it will be implemented)

@CanRau
Copy link

CanRau commented Feb 8, 2023

Yea I totally understand, was just curious if this was somehow already planned or sthg 😇

@caoimhebyrne
Copy link

It doesn't look like this is working, visiting a website deployed on Vercel with the following config gives an error in the function logs.

export const config = {
    isr: {
        expiration: 60,
    },
};
[GET] /
2023-02-13T04:45:17.515Z   dabb066b-5685-4a28-907b-cc1e1df327b3   ERROR   Error: Not found: /fn-0
    at resolve (file:///var/task/vercel/path0/.svelte-kit/output/server/index.js:3246:18)
    at resolve (file:///var/task/vercel/path0/.svelte-kit/output/server/index.js:3113:34)
    at #options.hooks.handle (file:///var/task/vercel/path0/.svelte-kit/output/server/index.js:3290:59)
    at respond (file:///var/task/vercel/path0/.svelte-kit/output/server/index.js:3111:43)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

dummdidumm added a commit that referenced this pull request Mar 21, 2023
Rich-Harris pushed a commit that referenced this pull request Apr 17, 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.

Route-level deployment config
6 participants