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

Support using stdout #725

Merged
merged 4 commits into from
Aug 26, 2024
Merged

Conversation

mzhubail
Copy link
Contributor

📑 Summary

Support writing to stdout if the user explicitly specifies -. In case of no output format, it is assumed to be svg and a warning is displayed.

Resolves #683

📏 Design Decisions

For the sake of making the code more readable, I'm using explicit '/dev/stdout'. This is just a personal preference and doesn't have anything to do with the platform.

I have also disabled writing to stdout if the input is markdown.

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 🔖 targeted master branch

If the user specifies `-` as the output, the code will be written to
`stdout` instead of writing to a file.

Note that for the sake of making the code more readable, I'm using
explicit `'/dev/stdout'`.  This is just a personal preference and
doesn't have anything to do with platform.
Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. I've got one minor comment that could potentially slightly simplify the code, but other than this, it looks great to me!

I'm not 100% sure, since it's been a while since I last programmed for Windows, but I think this should even work on Windows too!

src/index.js Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
@mzhubail
Copy link
Contributor Author

Forgot to mention that this feature is blocked by mermaid-js/zenuml-core#169, because the warning is written to stdout not stderr.

src/index.js Outdated
Comment on lines 169 to 171
const outputDir = path.dirname(output)
if (!fs.existsSync(outputDir)) {
error(`Output directory "${outputDir}/" doesn't exist`)
Copy link
Contributor Author

@mzhubail mzhubail Aug 20, 2024

Choose a reason for hiding this comment

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

Now that I think about it, maybe we shouldn't check the directory if we're writing to stdout.

Suggested change
const outputDir = path.dirname(output)
if (!fs.existsSync(outputDir)) {
error(`Output directory "${outputDir}/" doesn't exist`)
if (output !== '/dev/stdout') {
const outputDir = path.dirname(output)
if (!fs.existsSync(outputDir)) {
error(`Output directory "${outputDir}/" doesn't exist`)

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea.

Non-Linux operating systems probably won't have a /dev folder.

If you want to avoid having an extra level of indentation, we could also change the code to the following:

Suggested change
const outputDir = path.dirname(output)
if (!fs.existsSync(outputDir)) {
error(`Output directory "${outputDir}/" doesn't exist`)
const outputDir = path.dirname(output)
if (output !== '/dev/stdout' && !fs.existsSync(outputDir)) {
error(`Output directory "${outputDir}/" doesn't exist`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Forgot to mention that this feature is blocked by mermaid-js/zenuml-core#169, because the warning is written to stdout not stderr.

Unfortunately, that's a pretty major issue! As a quick fix, I think we could instead write this message to stderr instead, by changing the following to use console.warn instead:

mermaid-cli/src/index.js

Lines 243 to 245 in 967768c

page.on('console', (msg) => {
console.log(msg.text())
})

I've also noticed that the recently released Mermaid v11 seems to be logging a lot more information, so it's probably worth having that change anyway!

Copy link
Member

@aloisklink aloisklink left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for making those changes!

It should be released soon with mermaid-cli v11 once #701 is reviewed and merged!

@aloisklink aloisklink merged commit eb90b62 into mermaid-js:master Aug 26, 2024
7 checks passed
@mzhubail
Copy link
Contributor Author

Thanks!

falgon added a commit to falgon/roki-web that referenced this pull request Aug 27, 2024
using mermaid-cli within markdown

This feature will be available once mermaid-cli v11 is released ( mermaid-js/mermaid-cli#725 (review) ), so it still fails for now.
KeyWeeUsr added a commit to KeyWeeUsr/mermaid-docker-mode that referenced this pull request Dec 3, 2024
Ref:
* mermaid-js/mermaid-cli#725
* https://hub.docker.com/r/minlag/mermaid-cli/tags

Dropping the whole image building, simply using the upstream pre-built image
with options to specify own images and tags. Didn't work on MacOS anyway
because `google-chrome-stable` was unavailable on `linux/arm64` platform when
building the image. The upstream image supports `linux/arm64` out-of-box, so it
*should* work on MacOS.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write output to stdout
2 participants