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

Update CMD to serve instead of run; remove deps.ts in favor of deno.json #440

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kimdcottrell
Copy link

This fixes this issue:

#424

You can fix that issue as well by keeping the CMD as is, but you have to alter the main.ts to be:

Deno.serve(
    { port: 1993, hostname: "0.0.0.0" }, 
    (_req) => new Response("Hello, world")
);

I went with deno serve since you can keep all the hostname and port configs in the Dockerfile and reduce repetition.

deps.ts was removed in favor of demo.json as it, or package.json , seems to be the path forward in Deno 2.

@CLAassistant
Copy link

CLAassistant commented Jan 3, 2025

CLA assistant check
All committers have signed the CLA.

COPY deps.ts .
RUN deno install --entrypoint deps.ts
# Cache the dependencies as a layer (the following two steps are re-run only when deno.json is modified).
# Ideally cache deno.json will download and compile _all_ external files used in main.ts.
Copy link
Member

Choose a reason for hiding this comment

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

This is not preferable. With deno cache on like 62, we are going to install only dependencies that are actually used. Here all deps are installed, even if they are unused. We should wait for denoland/deno#27229

Copy link
Author

Choose a reason for hiding this comment

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

I agree. The dependency install and the caching of it should be sorted first.

The main reason I made this PR was to overcome an issue around the CMD and how the webserver was running, which would throw an error thanks to the approach it was using.

The deps.ts -> deno.json blurb got caught up in this since that also appears to be a remnant from Deno 1.

I'll try to keep an eye on your PR, and once it's merged, I'll alter this one to reflect your approach.

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.

3 participants