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

stream-whatwg: add whatwg streams #22352

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
5 changes: 5 additions & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,11 @@ module.exports = {
BigInt: false,
BigInt64Array: false,
Copy link
Member

Choose a reason for hiding this comment

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

I'm definitely not convinced that these should be globals right out the gate. Similar to URL, TextDecoder, and TextEncoder, making them non-global at least while they are still experimental makes sense.

Copy link
Member Author

@devsnek devsnek Aug 20, 2018

Choose a reason for hiding this comment

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

only the added node stream apis are experimental. the whatwg stream implementations are not experimental.

Copy link
Member

Choose a reason for hiding this comment

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

As we've done with the introduction of all WHATWG APIs, this entire implementation should land as experimental first until we're absolutely certain that it's definitely stable and has some use.

Copy link
Member Author

@devsnek devsnek Aug 20, 2018

Choose a reason for hiding this comment

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

the implementation is just copy-pasted from chromium, its almost 3 years old at this point, and has more tests than most stuff in node.js core. i see no reason to consider it experimental by that measurement. I have marked the APIs I wrote as experimental

i have some reservations around firing experimental warnings on access for them but if others also care i can add it in.

BigUint64Array: false,
ReadableStream: false,
WritableStream: false,
TransformStream: false,
CountQueuingStrategy: false,
ByteLengthQueuingStrategy: false,
COUNTER_HTTP_CLIENT_REQUEST: false,
COUNTER_HTTP_CLIENT_RESPONSE: false,
COUNTER_HTTP_SERVER_REQUEST: false,
Expand Down
9 changes: 8 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,10 @@ endif
v8:
tools/make-v8.sh $(V8_ARCH).$(BUILDTYPE_LOWER) $(V8_BUILD_OPTIONS)

.PHONY: wpt
wpt: all
$(NODE) ./test/wpt.js

.PHONY: jstest
jstest: build-addons build-addons-napi ## Runs addon tests and JS tests
$(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) \
Expand All @@ -273,6 +277,7 @@ test: all ## Runs default tests, linters, and builds docs.
$(MAKE) -s build-addons-napi
$(MAKE) -s cctest
$(MAKE) -s jstest
$(MAKE) -s wpt

.PHONY: test-only
test-only: all ## For a quick test, does not run linter or build docs.
Expand Down Expand Up @@ -453,6 +458,7 @@ test-ci: | clear-stalled build-addons build-addons-napi doc-only
$(PYTHON) tools/test.py $(PARALLEL_ARGS) -p tap --logfile test.tap \
--mode=$(BUILDTYPE_LOWER) --flaky-tests=$(FLAKY_TESTS) \
$(TEST_CI_ARGS) $(CI_JS_SUITES) $(CI_NATIVE_SUITES) $(CI_DOC)
$(NODE) test/wpt.js --tap
@echo "Clean up any leftover processes, error if found."
ps awwx | grep Release/node | grep -v grep | cat
@PS_OUT=`ps awwx | grep Release/node | grep -v grep | awk '{print $$1}'`; \
Expand Down Expand Up @@ -1097,7 +1103,8 @@ endif
LINT_MD_TARGETS = src lib benchmark test tools/doc tools/icu
LINT_MD_ROOT_DOCS := $(wildcard *.md)
LINT_MD_MISC_FILES := $(shell find $(LINT_MD_TARGETS) -type f \
-not -path '*node_modules*' -name '*.md') $(LINT_MD_ROOT_DOCS)
-not -path '*node_modules*' -not -path '*fixtures*' -name '*.md') \
$(LINT_MD_ROOT_DOCS)
run-lint-misc-md = tools/remark-cli/cli.js -q -f $(LINT_MD_MISC_FILES)
# Lint other changed markdown files maintained by us
tools/.miscmdlintstamp: $(LINT_MD_MISC_FILES)
Expand Down
11 changes: 11 additions & 0 deletions common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@

'openssl_fips%': '',

'v8_extra_library_files': [
'./lib/v8_extras/ByteLengthQueuingStrategy.js',
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these files consumed by the build? I see that gypfiles in deps/v8 references v8_extra_library_files, are you relying on the compilation of v8 to grab these lib/v8_extra js files and compile them in? That will be more awkward for node-chakracore to work with.

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 exactly what is happening. The V8 build compiles the v8_extras in. They are not loaded the same way as every other thing in core.

Copy link
Member Author

Choose a reason for hiding this comment

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

can chakra not just put these files somewhere and eval them when a context is created? we do plenty of stuff elsewhere that chakra has to shim.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, chakra shims a lot of things, but so far it has been native APIs (or js APIs). Not side effects of the build system. Sure, it can be done, but I'd much prefer not to.

Personally I also feel like this makes the build of node more complex to reason about, it is that much less of a dependency tree and more of a dependency graph.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you wanted to have these JS files live in lib and be compiled to native, then my preferred approach would be to explicitly put that logic in node's build, not use a side effect of v8's build.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MSLaguana are you saying to just put the list in node.gyp instead of common.gypi?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not the list, but the thing that consumes the list. The bit that takes the list of filenames and processes it to the point where the relevant build artifact is produced.

Copy link
Member

Choose a reason for hiding this comment

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

Looking through the implementation, I'm not sure which aspects of it would be infeasible to implement as a normal module. Doing so would make it significantly easier to polyfill, would make it more likely that it could just be dropped in to readable-stream, and wouldn't require any changes to the build system or any coordination between v8 and chakra-core. @devsnek ... can you explain a bit more about why you think such a port wouldn't be feasible?

Copy link
Member Author

Choose a reason for hiding this comment

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

v8 consumes the list and turns it into a c++ file that is linked internally to the engine. chakrashim would need to do something similar, calling those extras functions when new contexts are created.

Copy link
Member Author

@devsnek devsnek Aug 20, 2018

Choose a reason for hiding this comment

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

@jasnell because its a lot of stuff to rewrite (i don't have the time to do that) and i'd like to be able to collaborate with chromium by keeping at least similar implementations. readable-stream can use any of the polyfills on npm or even vendor the reference implementation from the streams repo.

'./lib/v8_extras/CommonOperations.js',
'./lib/v8_extras/CommonStrings.js',
'./lib/v8_extras/CountQueuingStragety.js',
'./lib/v8_extras/ReadableStream.js',
'./lib/v8_extras/SimpleQueue.js',
'./lib/v8_extras/TransformStream.js',
'./lib/v8_extras/WritableStream.js',
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is a straight up copy of the chromium code, it may be better to have this in deps than lib...

],

# Default to -O0 for debug builds.
'v8_optimized_debug%': 0,

Expand Down
62 changes: 62 additions & 0 deletions doc/api/stream.md
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,23 @@ reader.pipe(writer);
reader.unpipe(writer);
```

##### writable.acquireStandardStream()
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about calling these "...StandardStream"... Don't really have a better suggestion right now tho

Copy link
Member

Choose a reason for hiding this comment

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

This will definitely need more documentation about the new objects

Copy link
Member

Choose a reason for hiding this comment

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

Another thing to keep in mind is that the readable-streamsmodule will need to be able to implement support for this at some point. While that lags, these methods will not be implemented and will not be supported in an extremely large number of streams out there in the wild. It might be better to consider exposing these differently.

const { toWHATWGStream } = require('stream')
const myStream = toWHATWGStream(fs.createWriteStream())

This should allow it to work with any object that implements the right methods and events.

Copy link
Member Author

@devsnek devsnek Aug 20, 2018

Choose a reason for hiding this comment

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

Another thing to keep in mind is that the readable-streamsmodule will need to be able to implement support for this at some point.

i don't really understand this. a polyfill of whatwg streams shouldn't have a concept of node streams.

Copy link
Member

Choose a reason for hiding this comment

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

The readable-streams module provides an independently usable implementation of Node.js streams. This is adding new methods to the base class of require('stream').Readable and require('stream').Writable that will not be present in the implementation provided by the readable-streams module, nor will it be possible to implement those methods for every browser currently supported by readable-streams. Streams created by readable-streams today can be used within Node.js just as if they were the built-in streams.

Copy link
Member

Choose a reason for hiding this comment

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

The point is this can be done in a way where this isn't even a problem by not extending the prototype.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure it can, but I'm trying to design a good API, not a easy-for-some-npm-module-to-not-worry-about API.

Copy link
Member

Choose a reason for hiding this comment

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

"good" is entirely subjective here. Others will likely have very different opinions of what is "good".

The "readable-streams" module is not just "some-npm-module", it's the most downloaded module in the npm ecosystem and is part of the Node.js project managed by the Streams WG. Given that, I'm very concerned about implementation decisions that directly impact it.

Yes, we're able to add features to Node.js core before they propagate down to the module, but we must do so carefully and deliberately, weighing each choice on the impact it may have.

Copy link
Member Author

@devsnek devsnek Aug 20, 2018

Choose a reason for hiding this comment

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

I'm hopeful we can come up with something that isn't importing separate functions. I want to have as little friction as possible for people wanting to use whatwg streams, as there are quite a lot of locations where the interop needs to happen. I'm willing to continue bikeshedding this for as long as we need to for something that works for everyone, as it would seem we both have constraints here.

Copy link
Member

Choose a reason for hiding this comment

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

There are some relevant comments here that are being buried a bit after rebasing... just linking them here so that they don't get lost in the conversation: 1ca4477#r211124305 and 1ca4477#r211131273

<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

* Returns: {WritableStream} to feed this stream.

```js
const fs = require('fs');

const stream = fs.createWriteStream('file').acquireStandardStream();
const writer = stream.getWriter();
writer.write('hi!');
```

##### writable.cork()
<!-- YAML
added: v0.11.2
Expand Down Expand Up @@ -827,6 +844,22 @@ If both `'readable'` and [`'data'`][] are used at the same time, `'readable'`
takes precedence in controlling the flow, i.e. `'data'` will be emitted
only when [`stream.read()`][stream-read] is called.

##### readable.acquireStandardStream()
<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

* Returns: {ReadableStream} to fully consume the stream.

```js
const fs = require('fs');

const stream = fs.createReadStream('file').acquireStandardStream();
const reader = stream.getReader();
```

##### readable.destroy([error])
<!-- YAML
added: v8.0.0
Expand Down Expand Up @@ -1266,6 +1299,26 @@ Examples of `Duplex` streams include:
* [zlib streams][zlib]
* [crypto streams][crypto]

##### duplex.acquireStandardStream
<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

* Returns {Object}
* `readable` {ReadableStream}
* `writable` {WritableStream}

Creates a WHATWG stream pair to represent this duplex stream.

```js
const stream = getDuplexSomehow();
const { readable, writable } = stream.acquireStandardStream();
readable.getReader();
writable.getWriter();
```

#### Class: stream.Transform
<!-- YAML
added: v0.9.4
Expand Down Expand Up @@ -2200,6 +2253,15 @@ by [`stream._transform()`][stream-_transform]. The `'end'` event is emitted
after all data has been output, which occurs after the callback in
[`transform._flush()`][stream-_flush] has been called.

##### transform.acquireStandardStream()
<!-- YAML
added: REPLACEME
-->

> Stability: 1 - Experimental

* Returns: {TransformStream}

#### transform.\_flush(callback)

* `callback` {Function} A callback function (optionally with an error
Expand Down
8 changes: 8 additions & 0 deletions lib/_stream_duplex.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ module.exports = Duplex;
const util = require('util');
const Readable = require('_stream_readable');
const Writable = require('_stream_writable');
const { emitExperimentalWarning } = require('internal/util');

util.inherits(Duplex, Readable);

Expand Down Expand Up @@ -66,6 +67,13 @@ function Duplex(options) {
}
}

Duplex.prototype.acquireStandardStream = function() {
emitExperimentalWarning('Duplex.acquireStandardStream');
const readable = Readable.prototype.acquireStandardStream.call(this);
const writable = Writable.prototype.acquireStandardStream.call(this);
return { readable, writable };
};

Object.defineProperty(Duplex.prototype, 'writableHighWaterMark', {
// making it explicit this property is not enumerable
// because otherwise some prototype manipulation in
Expand Down
23 changes: 23 additions & 0 deletions lib/_stream_readable.js
Original file line number Diff line number Diff line change
Expand Up @@ -988,6 +988,29 @@ Readable.prototype[Symbol.asyncIterator] = function() {
return new ReadableAsyncIterator(this);
};

Readable.prototype.acquireStandardStream = function() {
emitExperimentalWarning('Readable.acquireStandardStream');
return new ReadableStream({
start: (controller) => {
this.pause();
this.on('data', (chunk) => {
controller.enqueue(chunk);
this.pause();
});
this.once('end', () => controller.close());
this.once('error', (e) => controller.error(e));
},
pull: () => {
this.resume();
},
cancel: () => {
this.destroy();
},
}, {
highWaterMark: this.readableHighWaterMark,
});
};

Object.defineProperty(Readable.prototype, 'readableHighWaterMark', {
// making it explicit this property is not enumerable
// because otherwise some prototype manipulation in
Expand Down
27 changes: 27 additions & 0 deletions lib/_stream_transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const {
} = require('internal/errors').codes;
const Duplex = require('_stream_duplex');
const util = require('util');
const { emitExperimentalWarning } = require('internal/util');
util.inherits(Transform, Duplex);


Expand Down Expand Up @@ -202,6 +203,32 @@ Transform.prototype._destroy = function(err, cb) {
});
};

Transform.prototype.acquireStandardStream = function() {
emitExperimentalWarning('Transform.acquireStandardStream');
return new TransformStream({
start: (controller) => {
this.on('data', (chunk) => {
controller.enqueue(chunk);
});
this.once('end', () => controller.close());
this.once('error', (e) => controller.error(e));
},
transform: (chunk) => {
return new Promise((resolve) => {
const underHighWaterMark = this.write(chunk);
if (!underHighWaterMark) {
this.once('drain', resolve);
} else {
resolve();
}
});
},
flush: () => {
this.end();
},
});
};


function done(stream, er, data) {
if (er)
Expand Down
27 changes: 27 additions & 0 deletions lib/_stream_writable.js
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,33 @@ Writable.prototype.end = function(chunk, encoding, cb) {
return this;
};

Writable.prototype.acquireStandardStream = function() {
Copy link
Member

Choose a reason for hiding this comment

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

How are errors propagated here? There's nothing listening to the error event.

internalUtil.emitExperimentalWarning('Writable.acquireStandardStream');
return new WritableStream({
start: (controller) => {
this.once('error', (e) => controller.error(e));
},
write: (chunk) => {
return new Promise((resolve) => {
const underHighWaterMark = this.write(chunk);
if (!underHighWaterMark) {
this.once('drain', resolve);
} else {
resolve();
}
});
},
close: (controller) => {
this.end();
},
abort: (reason) => {
this.destroy(reason);
},
}, {
highWaterMark: this.writableHighWaterMark,
});
};

Object.defineProperty(Writable.prototype, 'writableLength', {
// making it explicit this property is not enumerable
// because otherwise some prototype manipulation in
Expand Down
13 changes: 13 additions & 0 deletions lib/v8_extras/.eslintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/* eslint-disable no-restricted-globals */

const globals = new Set(Object.getOwnPropertyNames(global));
globals.delete('undefined');

module.exports = {
extends: ['../.eslintrc.yaml'],
rules: {
'strict': ['error', 'function'],
'no-restricted-syntax': 'off',
'no-restricted-globals': [2, ...globals],
},
};
55 changes: 55 additions & 0 deletions lib/v8_extras/ByteLengthQueuingStrategy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
//
// Redistribution and use in source and binary forms, with or without
// modification, are permitted provided that the following conditions are
// met:
//
// * Redistributions of source code must retain the above copyright
// notice, this list of conditions and the following disclaimer.
// * Redistributions in binary form must reproduce the above
// copyright notice, this list of conditions and the following disclaimer
// in the documentation and/or other materials provided with the
// distribution.
// * Neither the name of Google Inc. nor the names of its
// contributors may be used to endorse or promote products derived from
// this software without specific prior written permission.
//
// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

// eslint-disable-next-line no-unused-vars
(function(global, binding, v8) {
'use strict';

const defineProperty = global.Object.defineProperty;

class ByteLengthQueuingStrategy {
constructor(options) {
defineProperty(this, 'highWaterMark', {
value: options.highWaterMark,
enumerable: true,
configurable: true,
writable: true
});
}
size(chunk) {
return chunk.byteLength;
}
}

defineProperty(global, 'ByteLengthQueuingStrategy', {
Copy link
Member

Choose a reason for hiding this comment

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

These should likely emit an experimental process warning on first access.

Copy link
Member Author

Choose a reason for hiding this comment

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

this api is not experimental

Copy link
Member

Choose a reason for hiding this comment

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

See above. I'd want to land this entire implementation as experimental first.

value: ByteLengthQueuingStrategy,
enumerable: false,
configurable: true,
writable: true
});
});
Loading