Skip to content

Conversation

@magicxor
Copy link
Owner

see yt-dlp/yt-dlp#15012

With the release of yt-dlp version 2025.11.12, external JavaScript runtime support has arrived.
All users who intend to use yt-dlp with YouTube are strongly encouraged to install one of the supported JS runtimes.

see yt-dlp/yt-dlp#15012

With the release of yt-dlp version 2025.11.12, external JavaScript runtime support has arrived.
All users who intend to use yt-dlp with YouTube are strongly encouraged to install one of the supported JS runtimes.
Copilot AI review requested due to automatic review settings November 14, 2025 02:15
@magicxor magicxor self-assigned this Nov 14, 2025
@magicxor magicxor added the enhancement New feature or request label Nov 14, 2025
@magicxor magicxor merged commit 3784745 into master Nov 14, 2025
5 checks passed
@magicxor magicxor deleted the feature/nodejs branch November 14, 2025 02:16
Copilot finished reviewing on behalf of magicxor November 14, 2025 02:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds Node.js runtime support to the Docker container to comply with yt-dlp version 2025.11.12 requirements for YouTube downloads. The implementation uses fnm (Fast Node Manager) to install the LTS version of Node.js.

Key Changes:

  • Installs Node.js LTS using fnm (Fast Node Manager)
  • Changes the default shell to enable fnm's bash login initialization
  • Adds the unzip package to system dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +24 to +26
curl -o- https://fnm.vercel.app/install | bash
SHELL ["/bin/bash", "--login", "-c"]
RUN fnm install --lts && \
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The SHELL change to use --login shell affects all subsequent RUN commands in the Dockerfile, including the poetry installation at line 34. This could cause unexpected behavior or performance issues as login shells source additional configuration files.

Consider one of these approaches:

  1. Revert SHELL after the Node.js installation is complete
  2. Set up fnm's PATH manually without requiring a login shell
  3. Use a single RUN command with explicit shell invocation for just the fnm commands

Example for approach 2:

RUN apt-get update && \
    apt-get upgrade -y && \
    apt-get install -y curl ffmpeg unzip && \
    curl -o- https://fnm.vercel.app/install | bash && \
    export PATH="/root/.local/share/fnm:$PATH" && \
    eval "$(fnm env)" && \
    fnm install --lts && \
    node -v && npm -v && \
    python -m pip install --upgrade pip && \
    apt-get clean && rm -rf /var/lib/apt/lists/* && \
    mkdir /app
Suggested change
curl -o- https://fnm.vercel.app/install | bash
SHELL ["/bin/bash", "--login", "-c"]
RUN fnm install --lts && \
curl -o- https://fnm.vercel.app/install | bash && \
export PATH="/root/.local/share/fnm:$PATH" && \
eval "$(fnm env)" && \
fnm install --lts && \

Copilot uses AI. Check for mistakes.
Comment on lines +26 to 31
RUN fnm install --lts && \
node -v && \
npm -v && \
python -m pip install --upgrade pip && \
apt-get install -y curl ffmpeg && \
apt-get clean && rm -rf /var/lib/apt/lists/* && \
mkdir /app
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

Splitting the RUN command into two separate RUN statements creates an additional Docker layer and prevents proper cleanup. The apt-get clean && rm -rf /var/lib/apt/lists/* on line 30 runs in a separate layer from the apt-get install on line 23, so it doesn't actually reduce the image size.

Docker layers are immutable, so removing files in a later layer doesn't remove them from earlier layers. To effectively reduce image size, the installation and cleanup should be in the same RUN command.

Consider combining these RUN commands back together as suggested in the previous comment.

Copilot uses AI. Check for mistakes.
Comment on lines +26 to +28
RUN fnm install --lts && \
node -v && \
npm -v && \
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The fnm installation adds Node.js to the build environment but doesn't ensure it's available in the runtime environment. The fnm tool modifies the shell's PATH through .bashrc, which works with the --login shell, but this doesn't guarantee Node.js will be available when the container runs the CMD.

To ensure Node.js is available at runtime, add:

ENV PATH="/root/.local/share/fnm:$PATH"

after the fnm installation, or configure fnm to install Node.js to a permanent location that's added to the PATH environment variable.

Copilot uses AI. Check for mistakes.
ENV PH_VIDEO_DURATION=10
ENV MEDIA_CHAT_ID=-1002389753204
ENV RATE_LIMIT_WINDOW_MINUTES=1

Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The unzip package is added but there's no clear indication in the PR description or code why it's needed. If it's a dependency for fnm or Node.js installation, please document this in a comment. If it's unrelated to the Node.js installation, consider adding it in a separate commit or PR for clarity.

Suggested change
# 'unzip' is required for fnm/Node.js installation scripts

Copilot uses AI. Check for mistakes.
SHELL ["/bin/bash", "--login", "-c"]
RUN fnm install --lts && \
node -v && \
npm -v && \
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Extra trailing whitespace after npm -v. This should be removed for consistency.

Suggested change
npm -v && \
npm -v && \

Copilot uses AI. Check for mistakes.
magicxor added a commit that referenced this pull request Nov 14, 2025
magicxor added a commit that referenced this pull request Nov 14, 2025
* Revert "install node.js in docker container (#87)"

This reverts commit 3784745.

* use Deno runtime

Adds the Deno runtime to the Docker image.
This allows for the execution of Javascript and Typescript code directly within the application's environment.
Also installs unzip to allow extraction of zipped files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants