Skip to content

Correct compile errors#15

Open
tcely wants to merge 30 commits intokikkia:masterfrom
tcely:tcely-ejs
Open

Correct compile errors#15
tcely wants to merge 30 commits intokikkia:masterfrom
tcely:tcely-ejs

Conversation

@tcely
Copy link
Copy Markdown
Contributor

@tcely tcely commented Nov 26, 2025

PR Type

Enhancement, Other


Description

  • Migrate from local ejs submodule to esm.sh CDN import

  • Update all imports to use JSR and deno.json mappings

  • Simplify Dockerfile by removing ejs patching and git operations

  • Improve cache directory handling with fallback logic

  • Update to Debian 13 distroless image with tini init system


Diagram Walkthrough

flowchart LR
  A["Local ejs submodule"] -->|"Replace with"| B["esm.sh CDN import"]
  C["Scattered imports"] -->|"Consolidate via"| D["deno.json mappings"]
  E["Complex Dockerfile"] -->|"Simplify with"| F["Debian 13 + tini"]
  G["Hardcoded cache paths"] -->|"Improve with"| H["Fallback logic"]
Loading

Manual summary

  • Used a submodule for ejs1
  • Added imports to deno.json
  • Configured compiler options for worker.ts

Footnotes

  1. More detailed information: https://git-scm.com/book/en/v2/Git-Tools-Submodules

@tcely tcely changed the title Add a submodule for ejs Correct compile errors Nov 26, 2025
@PSTechiee
Copy link
Copy Markdown

PSTechiee commented Nov 26, 2025

Hey @kikkia

In readme.md make docker-compose up command TO docker compose up

- Specify Debian for builder
- Use `tini` for proper signals
- Create appropriate cache directories
@tcely
Copy link
Copy Markdown
Contributor Author

tcely commented Nov 26, 2025

See #16 for that change.

@tcely tcely force-pushed the tcely-ejs branch 6 times, most recently from ac93bcc to f6e4a7e Compare November 27, 2025 05:51
@kikkia
Copy link
Copy Markdown
Owner

kikkia commented Nov 29, 2025

Hey I am trying to get some time to mess with the submodule stuff so I am more familiar before merging it in. Do you think you could break that out from the other stuff so we have a PR to submodule ejs separate to the rest? Then we can merge that in, and when I can mess with the submodules and get more familiar I can merge in that stuff?

@tcely
Copy link
Copy Markdown
Contributor Author

tcely commented Nov 29, 2025

Are you looking to merge the rest of this pull request without using a submodule?
(I needed 3 files to make this work. #20)

Or are you looking to only merge the submodule change?
(I needed 5 files to make this work. #19)

Copy link
Copy Markdown

@PadowYT2 PadowYT2 left a comment

Choose a reason for hiding this comment

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

I'm not the most familiar with Deno, but I think it would be much better if you would add ejs to deno.json. With Bun you could you could just do github:yt-dlp/ejs@2655b1f55f98e5870d4e124704a21f4d793b4e1c. But as this project is for Deno, I believe using https://esm.sh would be better than managing a submodule (e.g. https://esm.sh/gh/yt-dlp/ejs@2655b1f55f98e5870d4e124704a21f4d793b4e1c/src/yt/solver/solvers.ts)

Also I would recommended pinning to a version and not a hash

@tcely
Copy link
Copy Markdown
Contributor Author

tcely commented Dec 1, 2025

I'm not the most familiar with Deno, but I think it would be much better if you would add ejs to deno.json. With Bun you could you could just do github:yt-dlp/ejs@2655b1f55f98e5870d4e124704a21f4d793b4e1c. But as this project is for Deno, I believe using https://esm.sh would be better than managing a submodule (e.g. https://esm.sh/gh/yt-dlp/ejs@2655b1f55f98e5870d4e124704a21f4d793b4e1c/src/yt/solver/solvers.ts)

Also I would recommended pinning to a version and not a hash

@PadowYT2
Would you please work up a pull request with the needed changes to the import lines?

Tip

The version tag corresponding to the currently pinned commit hash is: 0.3.0

@PadowYT2
Copy link
Copy Markdown

PadowYT2 commented Dec 1, 2025

I'm not the most familiar with Deno, but I think it would be much better if you would add ejs to deno.json. With Bun you could you could just do github:yt-dlp/ejs@2655b1f55f98e5870d4e124704a21f4d793b4e1c. But as this project is for Deno, I believe using esm.sh would be better than managing a submodule (e.g. esm.sh/gh/yt-dlp/ejs@2655b1f55f98e5870d4e124704a21f4d793b4e1c/src/yt/solver/solvers.ts)
Also I would recommended pinning to a version and not a hash

@PadowYT2 Would you please work up a pull request with the needed changes to the import lines?

Tip

The version tag corresponding to the currently pinned commit hash is: 0.3.0

Alright, I don't think it's possible to do it just because Deno requires a build entry and you can't just directly use the paths to the TypeScript files.

$ deno install
error: Module not found "https://esm.sh/gh/yt-dlp/ejs@0.3.0".

But you could just do import ... from "https://esm.sh/gh/yt-dlp/ejs@0.3.0/src/.../file.ts";, what do you think?

@tcely
Copy link
Copy Markdown
Contributor Author

tcely commented Dec 1, 2025

It's possible to do with an import map.

deno.json:

{
  "imports": {
    "@/ejs": "https://esm.sh/gh/yt-dlp/ejs@0.3.0?standalone",
    "@/ejs/": "https://esm.sh/gh/yt-dlp/ejs@0.3.0&standalone/"
  }
}

Then you can import files using:

import { ... } from "@/ejs/src/yt/solver/solvers.ts";

@PadowYT2 I look forward to seeing your pull request using this method.

@tcely
Copy link
Copy Markdown
Contributor Author

tcely commented Dec 1, 2025

@PadowYT2 Your suggested changes have been merged and I adjusted a few more things after the removal of the submodule.

@tcely tcely requested a review from PadowYT2 December 1, 2025 20:48
Comment thread deno.lock
Copy link
Copy Markdown

@PadowYT2 PadowYT2 left a comment

Choose a reason for hiding this comment

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

Would love to make a PR on this but unavailable as of right now. Also update README.md to remove the ejs installation

@tcely tcely requested a review from PadowYT2 December 1, 2025 21:08
Comment thread README.md
Comment thread README.md
Copy link
Copy Markdown

@PadowYT2 PadowYT2 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me and runs just fine. Although the && in the README is useless as cd should be fine here

@tcely
Copy link
Copy Markdown
Contributor Author

tcely commented Dec 6, 2025

@kikkia How are things looking? It'd be great to merge this now that a submodule isn't being used at all.

@kikkia
Copy link
Copy Markdown
Owner

kikkia commented Dec 8, 2025

Hey @tcely I am super busy recently, I have had very little free time to do much side project stuff. I will take a look as soon as I can, I should have time in the next 24 hours :)

Comment thread .github/workflows/main.yml Outdated
@tcely
Copy link
Copy Markdown
Contributor Author

tcely commented Dec 12, 2025

Hey @kikkia,

I will take a look as soon as I can, I should have time in the next 24 hours :)

I hope things have calmed for you. Let us know if there is anything more that can be done to let you merge these changes more easily.

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