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

Platform Abstraction / Cloudflare Worker Implementation #223

Closed
halfnelson opened this issue Dec 3, 2020 · 3 comments
Closed

Platform Abstraction / Cloudflare Worker Implementation #223

halfnelson opened this issue Dec 3, 2020 · 3 comments
Labels
adapters - general Support for functionality general to all adapters pkg:adapter-cloudflare-workers
Milestone

Comments

@halfnelson
Copy link
Contributor

I managed to get a cloudflare worker adapter working but it wasn't super smooth. I encountered the following problems, which we need to consider if we want to support javascript runtimes other than nodejs.

Dynamic Require Statements

The current code has a load option that hard coded during build to:

load: (route) => require(`./routes/${route.name}.js`);

The cloudflare worker environment (which is basically a web worker environment) doesn't support require. To get around this, cloudflare uses webpack, Webpack has issues with resolving that require.

Since we have the full manifest, we can optionally skip the dynamic require and generate code to static require the items.

    switch (route.name) {
       case 'main': return require('./routes/main.js');
       case 'article': return require('./routes/article.js');
       ....

Node specific modules

While cloudflare's builder uses webpack and can include node modules, the current renderer requires some which is node specific (fs, node-fetch, url). Luckily they are confined to the preload_context in renderer/page.js

To work around this there are a couple of options:

  • Adapters can use webpack's fallback functionality to provide shims
  • Svelte kit can be documented as explicitly nodejs environments only
  • We can provide a mechanism for the adapter to inject the required functionality

I had a stab at the third option by introducing the concept of a platform. A platform is a set (currently 3) of methods required by the svelte kit render function. It is passed in as part of the options (2nd parameter) to app.render.

In my implementation, SvelteKit provides a default node platform implementation that is used internally by dev, start, and prerender. When the render function is run from an adapter, the adapter needs to pass in a platform. It can just pass in the node platform:

const node_platform = require('@sveltejs/kit/assets/plaform/node')
const platform = node_platform(app.paths)

const rendered = await app.render({
		host: null, // TODO
		method: httpMethod,
		headers,
		path,
		query
	}, { platform });

or provide its own like the cloudflare worker one in the diff, which uses the global versions of response, and fetch available to the web worker scope, and the getAssetFromKV to fetch static files.

    const platform = {
	
    fetch: (url, init) => {
		return fetch(url, init)
    },

    fetch_file(pathname) {
		return getAssetFromKV(...)
    },

    create_response(body, options) {
        return new Response(body, options);
    }
}

The Question

The question is, is this something we want to pursue?

One disadvantage is that it will be yet another interface for adapter writers to be aware of, most which will be nodejs based, so this is just pure boilerplate for those.

Its advantage is that it would allow usage in different JS environments, such as web workers or embedded JS engines like in modern TVs. Being able to bundle an app up as a service worker might open up some pretty cool opportunities.

Is there a nicer way to do this?

Prototype:

The changes needed to be made have been prototyped in order to get the cf worker adapter going and can be seen in this commit:

2cac0f7

@benmccann
Copy link
Member

I think that supporting non-node platforms makes sense. There's a lot of code duplication between the adapters at the moment. It would be good to refactor these so that we have base classes or components that can be called. If there's a base class that provides the node platform then I don't think it would be any extra work for the existing adapters.

We may want to consider renaming the existing node-adapter if there's also a node-platform in order to clarify things. It might be better called node-server-adapter or something.

@Rich-Harris Rich-Harris added this to the public beta milestone Dec 4, 2020
@benmccann benmccann added the adapters - general Support for functionality general to all adapters label Dec 11, 2020
@Rich-Harris Rich-Harris modified the milestones: public beta, 1.0 Mar 19, 2021
@GrygrFlzr
Copy link
Member

Now that we have adapter-cloudflare-workers, is this still needed for e.g. Deno Deploy or for optimizing the existing adapters?

@benmccann
Copy link
Member

I think this was addressed by #1066. Please let me know if there's something here that we still would like to do and this should remain open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters - general Support for functionality general to all adapters pkg:adapter-cloudflare-workers
Projects
None yet
Development

No branches or pull requests

4 participants