Skip to content

[docker] Install tsc so that --emit-tsc works inside the container #1371

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.

Sign up for GitHub

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

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

[docker] Install tsc so that --emit-tsc works inside the container #1371

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
03ad9cb
feat(docker): install npm dependencies
merceyz Apr 12, 2024
aa033bb
Update docker/Dockerfile
sbc100 Apr 10, 2025
a32eef3
Update docker/Dockerfile
sbc100 Apr 10, 2025
f0b5785
Update docker/Dockerfile
sbc100 Apr 10, 2025
43f0af9
Merge branch 'main' into merceyz/docker-tsc
sbc100 Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ RUN echo "## Aggressive optimization: Remove debug symbols" \
&& find ${EMSDK}/upstream/bin -type f -exec strip -s {} + || true \
&& echo "## Done"

RUN echo "## Installing npm dependencies" \
Copy link
Collaborator

@sbc100 sbc100 Apr 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The emsdk build script uses does install npm dependencies, just no the devDependencies. It uses npm ci --production --no-optional.

How about emsdk install tsc so that we its more obvious why we are doing this here?

Copy link
Author

Choose a reason for hiding this comment

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

Hm, in that case perhaps emscripten should move typescript to dependencies instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We decided we didn't want to add the tsc dependency for all users, and instead make it an optional dependencies that certain users can opt into.

We can possibly revisit that decision at a later date.

Copy link

@s-h-a-d-o-w s-h-a-d-o-w Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Fair enough but why not merge this? Docker says the image is currently 2.76 GB. The difference between just production and all dependencies is ~70 MB.

(An example for something that's currently broken is compiling the tree-sitter WASM library with --emit-tsd.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just logically it makes sense that we are simply adding 'tsc'. Other dependencies should already exist. There is no need to install other dev dependencies here.

So maybe change the to "echo Installing" tsc and then npm install tsc?

Copy link

Choose a reason for hiding this comment

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

I'm not the original author, so I can't change it but that makes sense to me. (I'm not familiar with how emscripten uses TS, hence my broad suggestion. Just installing tsc is of course ideal.)

But if @merceyz isn't around any more (been inactive for a while), I'm happy to submit a new PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we just update this PR.. let me see if I can do that.

&& cd ${EMSDK} && . ./emsdk_env.sh \
&& cd ${EMSDK}/upstream/emscripten\
&& npm ci \
&& echo "## Done"

# ------------------------------------------------------------------------------
# -------------------------------- STAGE DEPLOY --------------------------------
# ------------------------------------------------------------------------------
Expand Down