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

Overhaul adapter API #2931

Merged
merged 108 commits into from
Dec 29, 2021
Merged

Overhaul adapter API #2931

merged 108 commits into from
Dec 29, 2021

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Nov 26, 2021

At present, svelte-kit build generates a monolithic app.js, via Vite. Adapters interact with it by importing the init and render functions therein and using them in the context of a platform-specific function — adapter-node calls render inside a Polka handler, adapter-netlify calls it inside a Lambda event handler, and so on.

This doesn't scale. Lambdas have a size limit of 50MB; Cloudflare Workers have just 1MB. Many apps won't reach these limits but many will. We need to be able to split apps into smaller chunks. Doing so has additional benefits:

  • startup is faster, in the case where code is bundled into a single script (e.g. workers)
  • routing should be faster, if a function only bundles a subset of the complete routing table, since we no longer need to evaluate n regexes inside SvelteKit to find the ones that match

This PR implements that idea, by updating the adapter API with a method to output multiple functions.

  • update adapter API to generate multiple functions
    • need to be able to return an array of entries, because of how Architect handles catch-alls (see below)
  • add a way to generate a manifest with all routes, for adapter-node etc
  • prevent functions being generated for prerendered pages (fixes Exclude prerendered pages from serverless functions/workers #1197)
  • update adapter-node
  • update and re-enable adapter-node tests
  • update adapter-vercel (this is partially blocked by some upcoming work on the filesystem API)
    • allow configuration of externals, so that lambdas can require native deps like sharp
  • update adapter-netlify
  • update adapter-cloudflare
    • expose prerendered pages
    • rethink how ASSETS is passed in; it's currently duplicative with the app manifest
  • rewrite adapter API documentation
  • remove sandbox from pnpm-workspace.yaml and lockfile
  • update adapter READMEs
  • add a utility for creating dirs inside .svelte-kit — hardcoding that directory feels super brittle
  • rename utils to builder, which still isn't a great name but is definitely better

It would also be nice to reach out to adapter authors who will be affected by these breaking changes.

Closes #2580 as well as the issues mentioned in that PR. I.e. closes #2517, closes #2581

Platform quirks

I think the API I've come up with, createEntries, is about as simple as it could be, though feedback is welcome. There's a fair bit of complexity inherent in this problem, because every platform does routing a little bit differently, and SvelteKit has two features in particular that are tricky to express on some of them:

  • Routing syntax is more flexible than most platforms:
    • Most platforms will only allow 'catch-all' or 'splat' parameters at the end of a route, rather than the middle — /foo/* is okay, /foo/*/bar is not
    • Rest parameters, the SvelteKit equivalent of a splat, accept 0 or more segments. Some platforms, e.g. Architect, interpret * as 1 or more segments, meaning that to express /foo/[...rest]/bar we need both /foo/* and /foo/bar routes pointing to the same function
    • foo/bar-[baz] will match /foo/bar-x but not /foo/qux-x. Most platforms only allow you to express /foo/:param
  • A page or endpoint can fall through to other routes. Most platforms have first-match-wins semantics, which means that a function may need to be responsible for multiple routes, and a route may appear in multiple functions.

In summary, a platform route needn't correspond to an app route, and the way that they're grouped will differ between platforms.

The createEntries method generates those groups:

const entries = utils.createEntries(route => {
  // for a file like `src/routes/foo/[bar].js` that exports `get` and `post`,
  // `route` would look like this:
  // 
  // {
  //   type: 'endpoint',
  //   pattern: /^foo/([^/]+)$/,
  //   segments: [
  //     { content: 'foo', dynamic: false, rest: false },
  //     { content: '[bar]', dynamic: true, rest: false }
  //   ],
  //   methods: ['get', 'post']
  // }

  return {
    // the `id` is used to uniquely identify a platform route.
    // for example, `src/routes/foo/a-[x]` and `src/routes/foo/b-[x]`
    // are both reduced to /foo/:param on some platforms, so we
    // discard the latter
    id: '...',
    
    // a function may need to handle multiple routes for two reasons:
    //   1. the platform's routing is less granular than SvelteKit's
    //   2. one route might fall through to another
    //
    // because of that, we need to know which (lower-precedence) app
    // routes should also be included in the function that handles
    // the main `route`. a route like `src/routes/[...all].svelte`
    // would be included in _every_ function
    filter: fallthrough => {
      return matches(route, fallthrough);
    },

    // once we have a group of routes that belong to an individual
    // function, we can create that function
    complete: entry => {
      const manifest = entry.generateManifest({
        relativePath: '../server'
      });

      const fn = `import { init } from '../handler.js';\n\nexport const handler = init(${manifest});\n`;

      writeFileSync(...);
      redirects.push(...);
    }
  }
});

It's a little complicated, but then it's doing a slightly complicated thing. Most SvelteKit users will never need to understand this or touch it.

Notable API changes

This felt like as good a time as any to introduce some overdue changes to the adapter API (though maybe it would be kind to deprecate certain methods instead of nuking them altogether).

We use snake_case inside the SvelteKit codebase, because it's inherently superior to camelCase. Since the wider JavaScript community isn't yet in agreement, our policy is to use camelCase for public APIs. Currently, the adapter API violates that principle.

In addition to adding createEntries, this PR updates the following method names:

  • copy_client_files -> writeClient
  • copy_server_files -> writeServer
  • copy_static_files -> writeStatic

The copy method signature has changed:

// old
utils.copy(from, to, file => file[0] !== '.');

// new
utils.copy(from, to, {
  filter: file => file[0] !== '.',
  replace: {
    ANSWER: '42'
  }
});

The app.js file still exists, but whereas it previously exported init and render functions, it now exports an App class that expects a manifest of the form created by the entry.generateManifest({...}) method above:

const fn = `
import { App } from './server/app.js';

const app = new App(${entry.generateManifest({ relativePath: './' })});

export const handler = (req) => {
  const [path, search] = req.url.split('?');

  const { status, headers, body } = app.render({
    method: req.httpMethod,
    headers: req.headers,
    path,
    query: new URLSearchParams(search),
    rawBody: normalise_body(req.body)
  });

  return {
    statusCode: status,
    headers,
    body
  };
};
`;

fs.writeFileSync(`.platform/functions/${entry.name}.js`, fn);

It also exports an override function, but this is used by SvelteKit during prerendering and svelte-kit preview; adapter authors don't need to use it.


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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Nov 26, 2021

🦋 Changeset detected

Latest commit: 0de1aa3

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

This PR includes changesets to release 8 packages
Name Type
@sveltejs/adapter-auto Patch
@sveltejs/adapter-cloudflare Patch
@sveltejs/adapter-cloudflare-workers Patch
@sveltejs/adapter-netlify Patch
@sveltejs/adapter-node Patch
@sveltejs/adapter-static Patch
@sveltejs/adapter-vercel Patch
@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

@matths
Copy link
Contributor

matths commented Jan 2, 2022

adapter-node calls render inside a Polka handler

So how does it work now?
When do you expect to have a documentation about the node-adapter and how to customize?
Is it still possible to use the whole SvelteKit App as just one middleware in another node.js Server application either with Polka, Express or whatever? Can you point me to an example, please?

@Mlocik97
Copy link
Contributor

Mlocik97 commented Jan 2, 2022

yes, Kit as middleware works fine and same way as it's written in docs.

@matths
Copy link
Contributor

matths commented Jan 3, 2022

@Mlocik97 can you point me to an example?
https://kit.svelte.dev/faq#how-do-i-use-x-with-sveltekit-how-do-i-use-middleware is an example how to use other middleware in DEV mode. No example about PROD mode.

https://kit.svelte.dev/docs#adapters-supported-environments-node-js
no more options beside out:

@Mlocik97
Copy link
Contributor

Mlocik97 commented Jan 3, 2022

it's partially described here: https://github.com/sveltejs/kit/tree/master/packages/adapter-node#custom-server

but I see it's missing in docs (it was there before), not sure why... but when they rewrote those adapter sections, somehow, part for this was deleted.

@abdo643-HULK
Copy link

it's partially described here: https://github.com/sveltejs/kit/tree/master/packages/adapter-node#custom-server

but I see it's missing in docs (it was there before), not sure why... but when they rewrote those adapter sections, somehow, part for this was deleted.

as far as I can remember it was only in the readme, like the current version

@Mlocik97
Copy link
Contributor

Mlocik97 commented Jan 3, 2022

if I remember correctly there was short description about it and link to Readme. It's not now... anyway it would be better to add it to docs.

@HalfdanJ
Copy link

HalfdanJ commented Jan 7, 2022

I'm starting porting over svelte-adapter-appengine to this new API, the serving part is pretty straight forward there (based on the adapter-node), but the routes are a bit more tricky. I need to create a routing file that correctly sends traffic either to static/prerendered content that is hosted from cloud storage (so never hitting the node server), or the dynamic traffic to the node server.

The new createEntries function seem to be a step forward in terms of creating this routing file correctly, but one thing I'm wondering about is how to determine in that function if a page is actually prerendered, and therefore shouldn't be handled by the node server. I could check if the .html file exists, but would it be possible to add a "prerendered": true to the entries in createEntries?

@geoffrich
Copy link
Member

@HalfdanJ builder.prerender() returns a list of prerendered files, which you should be able to use to determine if a page was prerendered.

prerender(options: { all?: boolean; dest: string; fallback?: string }): Promise<{
paths: string[];
}>;

@HalfdanJ
Copy link

HalfdanJ commented Jan 7, 2022

Aha ok that helps a bit,
I was looking at createEntries source, and I'm unsure if its intended that the prerendered paths even show up? I see this line https://github.com/sveltejs/kit/blob/master/packages/kit/src/core/adapt/builder.js#L66 that filters out prerendered paths, but the code above (facades) doesnt filter them out (https://github.com/sveltejs/kit/blob/master/packages/kit/src/core/adapt/builder.js#L41). Is that intended?

@jmsunseri
Copy link

pardon for asking here about this but how do i solve this issue not that the esbuild option is gone from the adapter config?
https://webcache.googleusercontent.com/search?q=cache%3AZPljxCZy8sYJ%3Ahttps%3A%2F%2Fwww.reddit.com%2Fr%2Fsveltejs%2Fcomments%2Frndsgu%2Fsolution_to_weird_error_when_trying_to_use_pg%2F+&cd=3&hl=en&ct=clnk&gl=tw

vite: {
			ssr: {
				noExternal: ['pg-admin']
			}
		}

has no effect on this issue.

@cryptodeal
Copy link

I second this ^^; My vercel build has also been failing since the removal of the esBuild config options in @sveltejs/adapter-vercel.

I've continued developing using the following package versions (last working):

"@sveltejs/adapter-vercel": "1.0.0-next.32",
"@sveltejs/kit": "1.0.0-next.206",

However, that's definitely less than ideal as I'm continuing to build around an outdated API for both @sveltejs/kit & @sveltejs/adapter-vercel.

The dependency causing my build issues (@napi-rs/*) is a package required indirectly as a dependency of a package in a shared monorepo used in the frontend. (Backend defines db schema, controllers, autogen type definitions, etc. that are used both in the frontend and for writing real time data scraped from various sources to the db, hence the monorepo greatly streamlines the dev process).

I'd previously built the frontend project successfully using both Vite resolve.alias and via installing the pnpm symlinked version of the built package as a dependency during the build process (setting pnpm to shamefully-hoist when building on Vercel) using the following esBuild config options:

adapter({
  esbuild(defaultOptions) {
    return {
      ...defaultOptions,
      plugins: [],
      external: ['@napi-rs/*']
    };
  }
})

Following the breaking API changes, I've tested with updated versions of @sveltejs/kit & @sveltejs/adapter-vercel using at least the following Vite config settings (both individually and in varying combinations), but to no avail.

Vite Config Options Tested:

  vite.ssr.noExternal: ['@napi-rs/*']
  ssr.external:  ['@napi-rs/*']
  vite.optimizeDeps.exclude: ['@napi-rs/*']

(I think I'd tried setting it as a rollup external as well using Vite config options, but that also failed to resolve the issue)

Build always fails with the following error:

error: No loader is configured for ".node" files

Any solutions for getting this build to work with the latest versions of @sveltejs/kit and @sveltejs/adapter-vercel?

@geoffrich
Copy link
Member

@jmsunseri @cryptodeal comments like these on merged PRs are easily lost. If this PR removed a feature you needed, can you open an issue instead?

@jmsunseri
Copy link

jmsunseri commented Jan 24, 2022

@jmsunseri @cryptodeal comments like these on merged PRs are easily lost. If this PR removed a feature you needed, can you open an issue instead?

I wasn't entirely sure it was an issue and maybe just me misunderstanding the correct path to take now that it's been removed. I had assumed there was a new better way to do it. If that's indeed not the case I will happily open a new issue.

@cryptodeal
Copy link

Good call, @geoffrich; I just opened Issue #3588.

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