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

Deno adapter #2934

Merged
merged 12 commits into from
Mar 30, 2022
Merged

Deno adapter #2934

merged 12 commits into from
Mar 30, 2022

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Mar 29, 2022

Changes

  • This adds a Deno adapter.
  • Changes code that depended on Node.js.
  • Moves the logger into core/logger and splits it between a console.log based implementation and a process.stdout/err based implementation. The latter runs in dev/build and the former in production SSR.

Testing

  • Test added for Deno in packages/integrations/deno/test/

Docs

  • Documented in the readme.

@changeset-bot
Copy link

changeset-bot bot commented Mar 29, 2022

🦋 Changeset detected

Latest commit: 50382f8

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

This PR includes changesets to release 2 packages
Name Type
astro Patch
@astrojs/deno 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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope) test 🚨 action Modifies GitHub Actions labels Mar 29, 2022
packages/astro/package.json Outdated Show resolved Hide resolved
@matthewp matthewp marked this pull request as ready for review March 29, 2022 17:12
@matthewp matthewp marked this pull request as draft March 29, 2022 17:15
@matthewp matthewp marked this pull request as ready for review March 29, 2022 18:36
Copy link
Member

@natemoo-re natemoo-re left a 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.

packages/astro/src/runtime/server/hydration.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@bholmesdev bholmesdev left a 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/core/logger/console.ts Outdated Show resolved Hide resolved
@@ -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;
Copy link
Member

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.

Copy link
Contributor Author

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'

Copy link
Member

@FredKSchott FredKSchott left a 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';
Copy link
Member

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.

Copy link
Member

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!

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

@okikio
Copy link
Member

okikio commented Mar 30, 2022

Yes!!!
YesBabyGIF

I am so excited for this

@matthewp matthewp merged commit 13b271b into main Mar 30, 2022
@matthewp matthewp deleted the bundle-all-deps-2 branch March 30, 2022 12:42
@github-actions github-actions bot mentioned this pull request Mar 30, 2022
alias: {
// This is needed for Deno compatibility, as the non-browser version
// of this module depends on Node `crypto`
'randombytes': 'randombytes/browser'
Copy link
Member

Choose a reason for hiding this comment

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

clever!

SiriousHunter pushed a commit to SiriousHunter/astro that referenced this pull request Feb 3, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 action Modifies GitHub Actions pkg: astro Related to the core `astro` package (scope) pkg: example Related to an example package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants