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.

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

Jump to bottom

does not work with Node's stream.pipeline() built-in #1

Closed
4 tasks done
derhuerst opened this issue Apr 19, 2023 · 4 comments
Closed
4 tasks done

does not work with Node's stream.pipeline() built-in #1

derhuerst opened this issue Apr 19, 2023 · 4 comments
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on

Comments

@derhuerst
Copy link

Initial checklist

Affected packages and versions

unified-stream@9, unified-stream@10

Link to runnable example

No response

Steps to reproduce

{
  "private": true,
  "name": "unified-stream-bug",
  "version": "1.0.0",
  "type": "module",
  "main": "index.js",
  "dependencies": {
    "rehype-format": "^4.0.1",
    "rehype-parse": "^8.0.4",
    "rehype-stringify": "^9.0.3",
    "unified": "^10.1.2",
    "unified-stream": "^2.0.0"
  }
}
// index.js
import {pipeline} from 'node:stream'
import {stream} from 'unified-stream'
import {unified} from 'unified'
import rehypeParse from 'rehype-parse'
import rehypeFormat from 'rehype-format'
import rehypeStringify from 'rehype-stringify'

pipeline(
  process.stdin,
  stream(
    unified()
    .use(rehypeParse)
    .use(rehypeFormat)
    .use(rehypeStringify)
  ),
  process.stdout,
  (err) => {
    if (!err) return;
    console.error(err)
    process.exit(1)
  },
)
echo -e 'foo\nbar' | node index.js

<html>
  <head></head>
  <body>
    foo
    bar
  </body>
</html>
file:///Users/j/playground/unified-stream-bug/node_modules/unified-stream/index.js:49
          throw new Error('Did not expect `write` after `end`')
          ^

Error: Did not expect `write` after `end`
    at write (file:///Users/j/playground/unified-stream-bug/node_modules/unified-stream/index.js:49:17)
    at EventEmitter.end (file:///Users/j/playground/unified-stream-bug/node_modules/unified-stream/index.js:90:9)
    at Socket.<anonymous> (node:internal/streams/pipeline:364:11)
    at Object.onceWrapper (node:events:627:28)
    at Socket.emit (node:events:525:35)
    at endReadableNT (node:internal/streams/readable:1359:12)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)

Node.js v19.4.0

Expected behavior

As unified-stream (claims to?) returns a duplex Node stream, I would expect to be able to use it in combination with stream.pipeline().

Actual behavior

It fails with an error, see the reproduction steps.

Affected runtime and version

[email protected]

Affected package manager and version

No response

Affected OS and version

No response

Build and bundle tools

No response
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Apr 19, 2023
@wooorm
Copy link
Member

wooorm commented Apr 20, 2023

Trying that locally on Node 19.8.1, everything works fine.

$ echo -e 'foo\nbar' | node example.js

<html>
  <head></head>
  <body>
    foo
    bar
  </body>
</html>

tilde in unified-stream on main [!?]
$ node -v
v19.8.1

Can you provide more info on OS/etc? Test more things?

@wooorm
Copy link
Member

wooorm commented Apr 20, 2023 •
edited
Loading

I can produce the error on Node 19.3.0 🤔 Weird! Not sure what happened there. But this project has been working for ages!

And it works again on Node 19.5

@wooorm
Copy link
Member

wooorm commented Apr 20, 2023

It looks like this: nodejs/node#46205.
Apparently there was a bug with Node calling end twice in 17.3+. I also can repro it on 18.2. It seems fixed in 19.5. I also see that it was backported to at least 18.16 (maybe earlier)?

I think it’s a clear bug in Node. Adding a workaround here for calling end twice to not throw, seems like it might silence more bugs.
So: recommend you update your engines! :)

@wooorm wooorm closed this as not planned Won't fix, can't repro, duplicate, stale Apr 20, 2023
@wooorm wooorm added the 👀 no/external This makes more sense somewhere else label Apr 20, 2023
@github-actions

This comment has been minimized.

Sign in to view
@github-actions github-actions bot added 👎 phase/no Post cannot or will not be acted on and removed 🤞 phase/open Post is being triaged manually labels Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Milestone
No milestone
Development

No branches or pull requests
2 participants
@wooorm @derhuerst