Skip to content

stream-whatwg: add whatwg streams #22352

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

Closed
devsnek wants to merge 3 commits into from
Closed

stream-whatwg: add whatwg streams #22352

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6000d44
stream-whatwg: add whatwg streams
devsnek Aug 20, 2018
d2e32a1
test: add wpt
devsnek Aug 19, 2018
e0117e7
streams: add whatwg stream interop
devsnek Aug 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,11 @@ module.exports = {
BigInt: false,
BigInt64Array: false,
Copy link
Copy Markdown
Member

@jasnell jasnell Aug 19, 2018

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
Copy Markdown
Member Author

@devsnek devsnek Aug 20, 2018 •
edited
Loading

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
Copy Markdown
Member

@jasnell jasnell Aug 20, 2018

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
Copy Markdown
Member Author

@devsnek devsnek Aug 20, 2018 •
edited
Loading

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
11 changes: 11 additions & 0 deletions common.gypi
View file
Open in desktop
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
Copy Markdown
Contributor

@MSLaguana MSLaguana Aug 20, 2018

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
Copy Markdown
Member

@jasnell jasnell Aug 20, 2018

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
Copy Markdown
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.

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
Copy Markdown
Contributor

@MSLaguana MSLaguana Aug 20, 2018

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
Copy Markdown
Contributor

@MSLaguana MSLaguana Aug 20, 2018

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
Copy Markdown
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.

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

Copy link
Copy Markdown
Contributor

@MSLaguana MSLaguana Aug 20, 2018

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
Copy Markdown
Member

@jasnell jasnell Aug 20, 2018

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
Copy Markdown
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.

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
Copy Markdown
Member Author

@devsnek devsnek Aug 20, 2018 •
edited
Loading

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
Copy Markdown
Member

@jasnell jasnell Aug 20, 2018

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
13 changes: 13 additions & 0 deletions lib/v8_extras/.eslintrc.js
View file
Open in desktop
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
View file
Open in desktop
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
Copy Markdown
Member

@jasnell jasnell Aug 20, 2018

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
Copy Markdown
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.

this api is not experimental

Copy link
Copy Markdown
Member

@jasnell jasnell Aug 20, 2018

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