-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Deno adapter #2934
Deno adapter #2934
Conversation
🦋 Changeset detectedLatest commit: 50382f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I had one comment about the serializeProps change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I deno if this works, but node-oubt I'm excited!
packages/astro/src/@types/astro.ts
Outdated
@@ -681,6 +682,7 @@ export interface AstroIntegration { | |||
'astro:server:start'?: (options: { address: AddressInfo }) => void | Promise<void>; | |||
'astro:server:done'?: () => void | Promise<void>; | |||
'astro:build:start'?: (options: { buildConfig: BuildConfig }) => void | Promise<void>; | |||
'astro:build:server:setup'?: (options: { vite: ViteConfigWithSSR }) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can this be astro:build:setup
to match the others? I'd really like to keep all of these to 3 words, max.
Since this appears to run on every build (not just SSR) I think it makes more sense as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed and added target: 'server' | 'client'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one naming comment!
After performing a build there will be a `dist/server/entry.mjs` module. You can start a server simply by importing this module: | ||
|
||
```js | ||
import './dist/entry.mjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we export a function that starts the server? Or, is the idea that this will also support running from deno run ./dist/entry.mjs
? I like to avoid side-effect imports like this, if we can help it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nvm i see the mention in the readme below. Interesting that we can support both!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case for the side-effect version is deploying straight to Deno Deploy without needing to add any code of your own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm
alias: { | ||
// This is needed for Deno compatibility, as the non-browser version | ||
// of this module depends on Node `crypto` | ||
'randombytes': 'randombytes/browser' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clever!
* Bundle everything, commit 1 * Get everything working * Remove dependency on readable-stream * Adds a changeset * Fix ts errors * Use the node logger in tests * Callback the logger when done writing * Fix test helper to await the callback * Use serialize-javascript again * Remove dead code * Rename hook * Oops
Changes
console.log
based implementation and aprocess.stdout/err
based implementation. The latter runs in dev/build and the former in production SSR.Testing
Docs