Skip to content

Nim: Add info about compiling to JS and running. - #2682

Open
moigagoo wants to merge 2 commits into
docker-library:masterfrom
moigagoo:nim_update_docs
Open

Nim: Add info about compiling to JS and running.#2682
moigagoo wants to merge 2 commits into
docker-library:masterfrom
moigagoo:nim_update_docs

Conversation

@moigagoo

Copy link
Copy Markdown
Contributor

No description provided.

@tianon tianon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks fine to me -- do you want to wait for your other PR to merge this, or merge this as-is since it's correct even without image changes?

(also one non-blocking note/question below)

Comment thread nim/content.md
RUN nim js -o:app.js src/app.nim

FROM node:latest
COPY --from=builder /usr/src/app/app.js .

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI, this is encoding the implicit assumption of WORKDIR /usr/src/app in the nim image (which is fine, but something you should be aware of / be doing intentionally if you're going to do it, since it makes that value slightly "stickier" to document it this way)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

True, and I was somewhat conflicted with this line... Initially, there was WORKDIR /app in both the builder and the runner images, which does make things more explicit but also noisier.

I think that as an example that is intended to be modified for your needs (I mean, you probably don't have an app called app anyway, duh) this is fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looks fine to me -- do you want to wait for your other PR to merge this, or merge this as-is since it's correct even without image changes?

I'd rather have that merged now as is.

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.

2 participants