stream: add Transform.by utility function (updated) #38696

safareli · 2021-05-15T11:30:55Z

Analogous to Readable.from, Transform.by creates transform streams from async function generators

instead of allocating a new function for the Promise constructor per call to next (e.g. via for await) allocate once on init and reuse

Co-Authored-By: Vse Mozhet Byt <[email protected]>

Co-Authored-By: Anna Henningsen <[email protected]>

Co-Authored-By: Vse Mozhet Byt <[email protected]>

…lity

Co-Authored-By: Anna Henningsen <[email protected]>

…ess.nextTick

Co-Authored-By: Vse Mozhet Byt <[email protected]>

Co-authored-by: Rich Trott <[email protected]>

nodejs-github-bot · 2021-06-04T07:45:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38486/

doc/api/stream.md

lib/internal/streams/transform.js

doc/api/stream.md

aduh95

To fix the linter errors

lib/internal/streams/transform.js

Co-authored-by: Antoine du Hamel <[email protected]>

mcollina

lgtm

mcollina · 2021-06-08T09:35:43Z

May I recommend a squash and force push?

nodejs-github-bot · 2021-06-08T09:36:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38527/

safareli · 2021-06-08T11:06:55Z

Sure, will do squash and push. Tho there is test failure here

We can either remove the test if supporting it is not important, or if we want to support it I guess something like this will do the trick:

Transform.by = function by(asyncGeneratorFn, options) {
  let { promise, resolve } = createDeferredPromise();
  const gen = (async function* () {
    while (true) {
      const { chunk, done, cb } = await promise;
      process.nextTick(cb);
      if (done) return;
      yield chunk;
      ({ promise, resolve } = createDeferredPromise());
    }
  })();
  Object.assign(gen, { encoding: undefined });
  return from(Duplex, asyncGeneratorFn(gen), {
    objectMode: true,
    autoDestroy: true,
    ...options,
    write: (chunk, encoding, cb) => {
      Object.assign(gen, { encoding });
      resolve({ chunk, done: false, cb });
    },
    final: (cb) => resolve({ done: true, cb }),
  });
};

What's your thought?

aduh95 · 2021-06-08T13:10:10Z

test/parallel/test-stream-transform-by.js

+  const readable = Readable.from('test');
+  async function * mapper(source) {
+    for await (const chunk of source) {
+      strictEqual(source.encoding, 'ascii');


Would that fix the test failure?

Suggested change

strictEqual(source.encoding, 'ascii');

strictEqual(chunk.encoding, 'ascii');

I guess point of the test is to make encoding of previous source stream available to the transformer, without transformer having to keep reference in some other ways. If we were to apply this change we might as well just remove the test.

mcollina · 2021-06-08T16:07:02Z

lib/internal/streams/transform.js

+    objectMode: true,
+    autoDestroy: true,
+    ...options,
+    write: (chunk, encoding, cb) => resolve({ chunk, done: false, cb }),


Suggested change

write: (chunk, encoding, cb) => resolve({ chunk, done: false, cb }),

write: (chunk, encoding, cb) => resolve({ chunk, encoding, done: false, cb }),

how about this?

this would just make encoding available here:

const { chunk, done, cb } = await promise;

safareli · 2021-06-08T17:48:14Z

test/parallel/test-stream-transform-by.js

+
+  const expected = ['T', 'E', 'S', 'T'];
+  for await (const chunk of stream) {
+    strictEqual(chunk, expected.shift());


Now this is failing:

python tools/test.py test/parallel/test-stream-transform-by.js === release test-stream-transform-by === Path: parallel/test-stream-transform-by node:internal/process/promises:246 triggerUncaughtException(err, true /* fromPromise */); ^ AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 'TEST' !== 'T' at Duplex.<anonymous> (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:130:5) at Duplex.emit (node:events:365:28) at addChunk (node:internal/streams/readable:314:12) at readableAddChunk (node:internal/streams/readable:289:9) at Duplex.Readable.push (node:internal/streams/readable:228:10) at next (node:internal/streams/from:98:31) at processTicksAndRejections (node:internal/process/task_queues:96:5) { generatedMessage: true, code: 'ERR_ASSERTION', actual: 'TEST', expected: 'T', operator: 'strictEqual' } Command: out/Release/node /Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js [00:00|% 100|+ 0|- 1]: Done

I suspect this was working with the original implementation.

I'm really not sure that's the issue :/

This test is wrong. If you want multiple chunks here you'd need to move from Readable.from('test') to use an array or split it yourself in multiplie chunks.

It's not just this but other tests are doing similar things. transformBy, transformByFuncReturnsObjectWithSymbolAsyncIterator to name a few (there are total 7 tests like this). And this tests are from the previous PR and I suspect It was passing there.

We changed the behavior of Readable.from() lately so that strings are not split char-by-char. This test must be changed.

@safareli any updates?

In the recent commit I've fixed some of the test errors but I'm still getting this which not sure how to fix and I gave up:

python tools/test.py test/parallel/test-stream-transform-by.js [00:00|% 0|+ 0|- 0]: release test-stream-trans === release test-stream-transform-by === Path: parallel/test-stream-transform-by Mismatched noop function calls. Expected exactly 1, actual 0. at mustCall (/Users/safareli/dev/deepchannel/node/test/common/index.js:350:10) at transformBySourceIteratorCompletes (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:64:21) at Object.<anonymous> (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:240:3) at Module._compile (node:internal/modules/cjs/loader:1109:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10) at Module.load (node:internal/modules/cjs/loader:989:32) at Function.Module._load (node:internal/modules/cjs/loader:829:14) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12) at node:internal/main/run_main_module:17:47 Mismatched <anonymous> function calls. Expected exactly 1, actual 0. at mustCall (/Users/safareli/dev/deepchannel/node/test/common/index.js:350:10) at transformByOnErrorAndTryCatchAndDestroyed (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:206:24) at Object.<anonymous> (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:247:3) at Module._compile (node:internal/modules/cjs/loader:1109:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10) at Module.load (node:internal/modules/cjs/loader:989:32) at Function.Module._load (node:internal/modules/cjs/loader:829:14) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12) at node:internal/main/run_main_module:17:47 Mismatched <anonymous> function calls. Expected exactly 1, actual 0. at mustCall (/Users/safareli/dev/deepchannel/node/test/common/index.js:350:10) at transformByThrowPriorToForAwait (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:229:22) at Object.<anonymous> (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:248:3) at Module._compile (node:internal/modules/cjs/loader:1109:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10) at Module.load (node:internal/modules/cjs/loader:989:32) at Function.Module._load (node:internal/modules/cjs/loader:829:14) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12) at node:internal/main/run_main_module:17:47 Mismatched noop function calls. Expected exactly 1, actual 0. at mustCall (/Users/safareli/dev/deepchannel/node/test/common/index.js:350:10) at Object.<anonymous> (/Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js:249:9) at Module._compile (node:internal/modules/cjs/loader:1109:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1138:10) at Module.load (node:internal/modules/cjs/loader:989:32) at Function.Module._load (node:internal/modules/cjs/loader:829:14) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12) at node:internal/main/run_main_module:17:47 Command: out/Release/node /Users/safareli/dev/deepchannel/node/test/parallel/test-stream-transform-by.js [00:00|% 100|+ 0|- 1]: Done

mcollina

Assigning the encoding to the generator is not correct as the encoding could potentially change mid-way via setEncoding(). The encoding needs to stay within each chunk. I hope my suggestions are enough to fix this issue.

lib/internal/streams/transform.js

safareli · 2021-06-09T16:39:17Z

looks like @sounisi5011 has similar lib stream-transform-from

sounisi5011 · 2021-06-09T19:25:40Z

looks like @sounisi5011 has similar lib stream-transform-from
#38696 (comment)

Yes, I created @sounisi5011/stream-transform-from because streams.Transform.from() did not exist in Node.js. However, if this will be built into Node.js, I would welcome it.

lib/internal/streams/transform.js

nodejs-github-bot · 2021-06-15T12:30:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/38636/

mcollina · 2021-06-17T14:15:48Z

Unfortunately there is a conflict now, could you update the PR?

ronag · 2022-01-03T15:34:16Z

Given the lack of progress here and that we landed stream.compose and Duplex.from which basically does this already I believe we can close this.

David Mark Clements and others added 26 commits December 16, 2019 12:00

stream: add Transform.by utility function

d957541

Analogous to Readable.from, Transform.by creates transform streams from async function generators

docs: stream.Transform.by typo

9005e46

stream: Transform.by SourceIterator next optimization

944f228

instead of allocating a new function for the Promise constructor per call to next (e.g. via for await) allocate once on init and reuse

docs: Transform.by doc tweaks

84c84c9

Co-Authored-By: Vse Mozhet Byt <[email protected]>

docs: sort type-parser types alphabetically within their groups

a1937c7

docs: sort type-parser types alphabetically within their groups

ad35382

docs: typo

8d82c5b

docs: Transform.by clarify as async iterable

7a1ef77

Co-Authored-By: Anna Henningsen <[email protected]>

stream: Transform. by remove unnecessary defensive code

5707445

stream: Transform.by code style

970ed3d

Co-Authored-By: Anna Henningsen <[email protected]>

stream: Transform.by minor refactoring

6e67a85

streams: Transform.by check fn return value instead of fn instance

98aebc5

docs: emphasize Transform.by objectMode default behaviour

eee19c5

docs: Transform.by function naming convention

26e96be

Co-Authored-By: Vse Mozhet Byt <[email protected]>

docs: add transform content to streams <-> async generators compatibi…

185a6f4

…lity

docs: includemissing parens

8e73c51

tests: preempt conflict with nodejs#28566

b73347f

tests: fix transform async iterator test

9789a5b

docs: add meta data to Transform.by

bad0bfd

Co-Authored-By: Anna Henningsen <[email protected]>

stream: error handling bug fix, ensure stream is destroyed after proc…

f2c8b22

…ess.nextTick

Update doc/api/stream.md

001fe01

Co-Authored-By: Vse Mozhet Byt <[email protected]>

Update doc/api/stream.md

9545ee0

Co-Authored-By: Vse Mozhet Byt <[email protected]>

Update doc/api/stream.md

f0fa8b6

Co-Authored-By: Vse Mozhet Byt <[email protected]>

Update doc/api/stream.md

b504b15

Co-Authored-By: Vse Mozhet Byt <[email protected]>

streams: Transform.by, rm unncessary check

447a895

lint fixes

2fe96e4

github-actions bot added doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem. labels May 15, 2021

Apply stream.md suggestions from code review

a985441

Co-authored-by: Rich Trott <[email protected]>

aduh95 reviewed Jun 4, 2021

View reviewed changes

apply @aduh95 suggestions

e0c1f0e

aduh95 reviewed Jun 6, 2021

View reviewed changes

doc/api/stream.md Outdated Show resolved Hide resolved

aduh95 reviewed Jun 6, 2021

View reviewed changes

lib/internal/streams/transform.js Outdated Show resolved Hide resolved

lib/internal/streams/transform.js Outdated Show resolved Hide resolved

safareli and others added 2 commits June 6, 2021 17:04

Apply suggestions from code review

c5c862d

Co-authored-by: Antoine du Hamel <[email protected]>

Update doc/api/stream.md

adc6059

Co-authored-by: Antoine du Hamel <[email protected]>

mcollina approved these changes Jun 8, 2021

View reviewed changes

aymen94 approved these changes Jun 8, 2021

View reviewed changes

aduh95 reviewed Jun 8, 2021

View reviewed changes

mcollina reviewed Jun 8, 2021

View reviewed changes

assign encoding to the async generator

967989e

safareli commented Jun 8, 2021

View reviewed changes

mcollina added notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 9, 2021

mcollina requested changes Jun 9, 2021

View reviewed changes

lib/internal/streams/transform.js Outdated Show resolved Hide resolved

lib/internal/streams/transform.js Outdated Show resolved Hide resolved

lib/internal/streams/transform.js Outdated Show resolved Hide resolved

add encoding to yield

6ccafab

thefourtheye reviewed Jun 14, 2021

View reviewed changes

lib/internal/streams/transform.js Show resolved Hide resolved

try fix some tests

9bccc23

mcollina mentioned this pull request Jul 1, 2021

stream: compose #39029

Closed

ronag closed this Jan 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: add Transform.by utility function (updated) #38696

stream: add Transform.by utility function (updated) #38696

safareli commented May 15, 2021

nodejs-github-bot commented Jun 4, 2021

aduh95 left a comment

mcollina left a comment

mcollina commented Jun 8, 2021

nodejs-github-bot commented Jun 8, 2021

safareli commented Jun 8, 2021

aduh95 Jun 8, 2021

safareli Jun 8, 2021

mcollina Jun 8, 2021

safareli Jun 8, 2021

safareli Jun 8, 2021

mcollina Jun 10, 2021

safareli Jun 10, 2021

mcollina Jun 11, 2021

mcollina Jun 15, 2021

safareli Jun 15, 2021

mcollina left a comment

safareli commented Jun 9, 2021 •

edited

Loading

sounisi5011 commented Jun 9, 2021 •

edited

Loading

nodejs-github-bot commented Jun 15, 2021

mcollina commented Jun 17, 2021

ronag commented Jan 3, 2022

	strictEqual(source.encoding, 'ascii');
	strictEqual(chunk.encoding, 'ascii');

	write: (chunk, encoding, cb) => resolve({ chunk, done: false, cb }),
	write: (chunk, encoding, cb) => resolve({ chunk, encoding, done: false, cb }),

stream: add Transform.by utility function (updated) #38696

stream: add Transform.by utility function (updated) #38696

Conversation

safareli commented May 15, 2021

nodejs-github-bot commented Jun 4, 2021

aduh95 left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jun 8, 2021

nodejs-github-bot commented Jun 8, 2021

safareli commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

safareli commented Jun 9, 2021 • edited Loading

sounisi5011 commented Jun 9, 2021 • edited Loading

nodejs-github-bot commented Jun 15, 2021

mcollina commented Jun 17, 2021

ronag commented Jan 3, 2022

safareli commented Jun 9, 2021 •

edited

Loading

sounisi5011 commented Jun 9, 2021 •

edited

Loading