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

WIP: Add adapter-deno #1271

Closed
wants to merge 2 commits into from
Closed

Conversation

jpaquim
Copy link
Contributor

@jpaquim jpaquim commented Apr 29, 2021

I just saw this issue #976, so please feel free to close this if there's no interest.

This is still a work-in-progress, but my goal was to have some kind of working adapter for Deno, or otherwise understand the main points when integrating with the current Svelte Kit code.

The good news is I managed to get a couple of examples working, the skeleton template, and the default template almost working.

The process I used for testing was setting up a new Svelte Kit app with npm init, npm linking my local adapter-deno, and using a process equivalent to adapter-node:

// svelte.config.cjs
const deno = require('@sveltejs/adapter-deno');

module.exports = {
  kit: {
    target: '#svelte',
    adapter: deno(),
  },
};

To actually run the built code with deno, I'm using

# deno run --import-map=import-map.json --allow-env --allow-read --allow-net build/index.js

where import-map.json maps the module names to the corresponding location in the filesystem:

{
	"imports": {
		"@sveltejs/kit/http": "./node_modules/@sveltejs/kit/dist/http.js",
		"@sveltejs/kit/ssr": "./node_modules/@sveltejs/kit/dist/ssr.js",
		"svelte": "./node_modules/svelte/index.mjs",
		"svelte/animate": "./node_modules/svelte/animate/index.mjs",
		"svelte/compiler": "./node_modules/svelte/compiler/index.mjs",
		"svelte/easing": "./node_modules/svelte/easing/index.mjs",
		"svelte/internal": "./node_modules/svelte/internal/index.mjs",
		"svelte/motion": "./node_modules/svelte/motion/index.mjs",
		"svelte/store": "./node_modules/svelte/store/index.mjs",
		"svelte/transition": "./node_modules/svelte/transition/index.mjs"
	}
}

To actually run the default Svelte Kit template, I had to adapt hooks.js to use the Deno version of uuid/v4, and removed the cookie-handling for now.

There's definitely still a lot of work to be done, and I have a feeling that I'll soon hit a roadblock, but still wanted to put this out there in case anyone else wants to do something with it.

@benmccann
Copy link
Member

I'm not at all familiar with Deno. I noticed you had to use their server package, which doesn't support compression, etc. Can Deno not run Polka?

It also seems a little annoying that Deno isn't capable of understanding the exports map in Kit's package.json forcing you to provide the import map. It looks like there's an open issue about that: denoland/deno#6782

Anyway, glad to see that it seems like this mostly works! I do think it'd probably be better for this to live in an external repo, but we should surface it somewhere like https://github.com/sveltejs/integrations

@jpaquim
Copy link
Contributor Author

jpaquim commented Apr 29, 2021

I'm also relatively new to working with Deno, and still struggling a bit with the Node-Deno interfacing and cross-compatibility issues.

I did not try to get polka or sirv working, although I assume it should be possible using an ESM build plus some kind of Node compatibility layer. I think most ESM CDNs like Skypack don't do any kind of Node-Deno polyfills, although https://esm.sh/ does seem to support it for certain packages. In any case, there's a node compatibility layer in the std library that can in principle be used to require() modules as usual: https://deno.land/std/node

Instead, I looked around for the HTTP servers available at https://deno.land/x/, in the spirit of trying to keep it aligned with Deno conventions, even if the ecosystem is still very much in flux. I used opine because it provides the same Express-like interface, and a static-serving middleware out of the box, it was just the fastest way to get something working.

The import map thing is indeed not ideal, although it might be possible to auto-generate it at build-time in adapt(). Will need to look into it.

@jpaquim
Copy link
Contributor Author

jpaquim commented Apr 29, 2021

The main problem I'm facing, though, is how to handle server-side code in Svelte components, API routes, or getContext/handle hooks? On one hand it needs to be imported and analysed by Node/Rollup at build-time, on the other hand it needs to run using Deno at runtime. It doesn't seem like it's possible to have both things play together well.

Looking at the other adapters, it seems like there is much less friction with this kind of issue, I guess because the underlying runtimes are Node-compatible. From what I understand, the same Svelte Kit project should be able to be built using any adapter, but this assumption seems to be naturally restricted to Node-compatible runtime adapters.

This is also kind of related to the discussion in #754 regarding support for Web-only ESM packages, the same friction occurs between the client and the server runtimes, due to universal rendering imposing some degree of cross-compatibility of packages between target environments.

@Rich-Harris
Copy link
Member

Rather than providing an import map etc, I wonder if we should just be bundling node_modules? In e.g. the Vercel adapter we're doing that with esbuild:

await esbuild.build({
entryPoints: ['.svelte/vercel/entry.js'],
outfile: join(dirs.lambda, 'index.js'),
bundle: true,
platform: 'node'
});
writeFileSync(join(dirs.lambda, 'package.json'), JSON.stringify({ type: 'commonjs' }));
. Presumably we need to exclude dependencies that are e.g. deno.land URLs

@jpaquim
Copy link
Contributor Author

jpaquim commented May 4, 2021

That does sound like a good option, the import map was mostly used as an interim solution, bundling is comparatively more resilient to a few of the cases I mentioned. I'll take a look at it later this week, looking at the esbuild API it seems like either the neutral or browser platform settings should cooperate well with Deno.

However, I'd love to hear your opinion @Rich-Harris concerning Svelte Kit's cross-adapter/runtime compatibility. Unless the Node compatibility layer is used for everything, it won't be possible in practice for a single SvelteKit application to support both Node-style adapters and the Deno adapter. Is that consistent with the philosophy you're aiming for?

@Rich-Harris
Copy link
Member

Unless the Node compatibility layer is used for everything, it won't be possible in practice for a single SvelteKit application to support both Node-style adapters and the Deno adapter. Is that consistent with the philosophy you're aiming for?

Can you elaborate? The SvelteKit runtime doesn't (or at least shouldn't) have any Node dependencies — in theory it will run anywhere JavaScript runs. (Of course, that doesn't take account of other app dependencies that use Node built-ins; presumably we'd need SvelteKit itself to run on Deno in order to prevent those sneaking in)

@jpaquim
Copy link
Contributor Author

jpaquim commented May 4, 2021

Exactly, I was thinking of external Node dependencies that will inevitably be used in real applications. In practice, once a non-pure JS dependency is needed (either Node or Deno), you have to commit to the corresponding runtime.

For a significant fraction of projects I guess it would be feasible to do something like starting with adapter-node, migrating to adapter-netlify, adapter-begin or adapter-vercel down the line, whereas to do the same for adapter-deno would require adapting all the Node-specific imports (which I guess is not too bad, if the Deno-supported Node-compatibility layer works well for these cases).

For someone starting with Deno today (and using Deno-specific libraries, URL imports, in their SSR code, hooks, etc.), migration to another runtime would be comparatively non-trivial, at least in the absence of a comparable Node-supported Deno compatibility layer.

That's kind of the gist of the argument, although your proposal for just bundling in the imports/requires does simplify matters for pure JS libraries, I'll try to put something together that at least successfully builds the Svelte Kit demo and Realworld apps.

@jpaquim jpaquim force-pushed the feat/adapter-deno branch 6 times, most recently from 6573742 to e045f6d Compare May 8, 2021 20:41
@jpaquim
Copy link
Contributor Author

jpaquim commented May 8, 2021

I made some further progress on this branch:

  • Adapted to new .js module files instead of .cjs
  • Added some documentation in the README.md, adapted from the new adapter-node docs
  • Used esbuild as you suggested @Rich-Harris to bundle the Svelte build-time dependencies, instead of using the import-map.json approach, seems to be working great 🚀
  • For runtime dependencies, I'm sticking to the deps.ts convention used in most Deno projects. By default the adapter will add its own deps.ts providing the opine application server, but a user-provided deps.ts can be used instead by specifying it in the adapter's options.deps config field.
  • The most persistent issue was down to the difference between Node's import('http').IncomingMessage type vs Deno's import('https://deno.land/std/http/mod.ts').ServerRequest, leading to having to provide Deno-specific version of the getRawBody() function.
  • And the same thing for the request headers, import('http').IncomingHttpHeaders vs the Deno/Browser standard Headers.

I did get the Svelte Kit demo application fully working, whereas before I couldn't get the /todos endpoints working correctly, and would definitely invite people to play around with this, especially given my limited experience with Deno.

The SvelteKit runtime doesn't (or at least shouldn't) have any Node dependencies — in theory it will run anywhere JavaScript runs. (Of course, that doesn't take account of other app dependencies that use Node built-ins; presumably we'd need SvelteKit itself to run on Deno in order to prevent those sneaking in)

It seems like there is indeed some subtle coupling with Node in a couple of functions, due to the way they are typed. getRawBody() is an example, and there's likely other examples in the public API of this type of dependency on Node's http types.

I guess it's always possible to argue that the caller is responsible for converting into these interfaces, but that does not necessarily provide the best experience in terms of dev ergonomics.

@Rich-Harris
Copy link
Member

It seems like there is indeed some subtle coupling with Node in a couple of functions, due to the way they are typed. getRawBody() is an example, and there's likely other examples in the public API of this type of dependency on Node's http types.

yeah, the @sveltejs/kit/http module should probably be @sveltejs/kit/node, since its purpose is to provide a helper(s) for Node-ish environments

return null;
}

const data = await Deno.readAll(req.body);

Choose a reason for hiding this comment

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

please consider changing this, see https://deno.com/blog/v1.9#new-api-deprecations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just saw that this morning, updated it to use the std/io readAll instead of the deprecated global Deno namespace one. Can you check again?

Copy link

@buttercubz buttercubz left a comment

Choose a reason for hiding this comment

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

LGTM, good job

@jpaquim
Copy link
Contributor Author

jpaquim commented May 26, 2021

@buttercubz what would you say would be the logical next step for this?

I was thinking of writing basic smoke tests (adapted from the other adapters), but I guess it would make sense to write it already under the Deno runtime and conventions, so using Deno.test() and deno test, but to make that compatible with the existing package.json test script would imply using something like https://github.com/kt3k/deno-bin right? Is it possible to add it as a devDependency without impacting all Svelte Kit devs?

Going down that route, I wonder whether it would make sense to use deno bundle as the bundler at some point. Currently this is using rollup at adapter build-time, and esbuild at project build-time, but in theory it should also build under deno bundle.

@buttercubz
Copy link

buttercubz commented Jun 7, 2021

@buttercubz what would you say would be the logical next step for this?

I was thinking of writing basic smoke tests (adapted from the other adapters), but I guess it would make sense to write it already under the Deno runtime and conventions, so using Deno.test() and deno test, but to make that compatible with the existing package.json test script would imply using something like https://github.com/kt3k/deno-bin right? Is it possible to add it as a devDependency without impacting all Svelte Kit devs?

Going down that route, I wonder whether it would make sense to use deno bundle as the bundler at some point. Currently this is using rollup at adapter build-time, and esbuild at project build-time, but in theory it should also build under deno bundle.

my apologies for the slow response, I think tests should be added, a good solution is https://www.npmjs.com/package/deno-bin, regarding the bundle I think rollup would be more than enough.

also we can build a deno deploy adapter, i think

Use esbuild to build server/app.js

Use Deno-specific version of getRawBody() to handle differences in
ServerRequest object types
@changeset-bot
Copy link

changeset-bot bot commented Jun 20, 2021

⚠️ No Changeset found

Latest commit: 221d868

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

@benmccann
Copy link
Member

@jpaquim it's really great that you shared this with us and awesome to see it progressing! We're trying to have most of the adapters maintained externally as we don't have enough expertise with the various platforms. We're going to launch a new version of sveltesociety.dev written in SvelteKit that has a section listing out the various SvelteKit adapters that have been created and link to that from the SvelteKit site. It'd be great to see this work get its own repo and listed there. You can send a PR to list adapter-deno when you're ready adding it the same way as the Firebase adapter: https://github.com/svelte-society/sveltesociety-2021/blob/544d76e29da5d9ea13d808e4b7e8306a93cbaf6d/src/routes/components/components.json#L3

I'm going to go ahead and close this, but please let us know if there are API issues you need to solve to support Deno

@benmccann benmccann closed this Jun 28, 2021
@jpaquim
Copy link
Contributor Author

jpaquim commented Jun 28, 2021

Thanks for the feedback @benmccann , I'll proceed as you mentioned and move this to a separate repository. Is there any community maintained organization where it would make sense to create the repo? Or should I just create it under my personal account/org? And I guess the naming convention for the npm package should be something like svelte-adapter-deno, right?

@benmccann
Copy link
Member

There's no community-maintained org, so putting it in your own account or org would make sense. svelte-adapter-deno sounds good as a name 😄

@jpaquim
Copy link
Contributor Author

jpaquim commented Jul 12, 2021

@benmccann @buttercubz and whoever may be interested, I moved the Deno adapter code to a new repo, and published an initial version to NPM:

Repo: https://github.com/pluvial/svelte-adapter-deno
NPM package: https://www.npmjs.com/package/svelte-adapter-deno

Everyone is welcome to join in on the development, next steps will be writing tests, and filling out the missing functionality such as cookie handling. And I've yet to try out Deno Deploy, there may be something interesting to do there, more similar to the Cloudflare Workers adapter.

@benmccann
Copy link
Member

Cool! I added it to sveltejs/integrations and to the Svelte Society rewrite (when the latter is done it will replace the former)

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.

4 participants