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

Rename App #4030

Closed
Rich-Harris opened this issue Feb 21, 2022 · 1 comment · Fixed by #4034
Closed

Rename App #4030

Rich-Harris opened this issue Feb 21, 2022 · 1 comment · Fixed by #4034

Comments

@Rich-Harris
Copy link
Member

Describe the problem

Internally, we have two things called App — the class that is created by svelte-kit build, and instantiated by adapters...

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

const app = new App(manifest);

// later
const response = await app.render(request);

...and the namespace for specifying app-level interfaces like locals:

declare namespace App {
  interface Locals {}
  interface Platform {}
  interface Session {}
  interface Stuff {}
}

This hasn't really been a problem, but now that we're adding docs for the types (#4011), it's going to become a source of confusion and broken links.

Describe the proposed solution

I think it makes more sense to rename the class than the namespace. The best suggestion so far is Server:

// created in build_server.js
export class Server {
  constructor(manifest) {
    // ...
  }
}
// created by an adapter
import { Server } from 'SERVER';
import { manifest } from 'MANIFEST';

const server = new Server(manifest);

// later
const response = await server.render(request);

At the risk of inviting bikeshedding, it might also make sense to change server.render to server.respond or server.handle or something, since 'render' sort of implies something side-effect free (which PUT/POST/DELETE requests aren't), and also implies that we're doing SSR when in some cases we're just rendering an empty shell page.

Aside: I've been meaning to overhaul Router and Renderer — having two separate classes seemed like a good idea at the time, but in practice it hasn't worked out, and it would make sense to unify them into a single class. There'd be a nice symmetry if we had a class Server and a class Client.

Alternatives considered

We could rename the namespace instead, though it's less clear what we could rename it to, and it would be a breaking change for more people (all Kit users rather than just adapter authors).

Importance

would make my life easier

Additional Information

No response

@Rich-Harris Rich-Harris mentioned this issue Feb 21, 2022
7 tasks
@mrkishi
Copy link
Member

mrkishi commented Feb 21, 2022

server.respond() seems appropriate as it returns a Promise<Response>. :)

There'd be a nice symmetry if we had a class Server and a class Client.

It'd also be nice to have the server in a runtime/server/server.js with built data in the manifest à la the client. The less generated code, the easier to navigate the codebase, imo, unless strictly necessary.

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 a pull request may close this issue.

2 participants