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

add sd-streams from https://github.com/stardazed/sd-streams/blob/mast… #3192

Merged
merged 12 commits into from Oct 28, 2019

Conversation

@nstott
Copy link
Contributor

nstott commented Oct 23, 2019

fixes #2828

@stardazed/streams was removed from third_party/ in https://github.com/denoland/deno/pull/2826/files

This adds sd streams from https://github.com/stardazed/sd-streams/tree/8928cf04b035fd02fb1340b7eb541c76be37e546 to cli/js/streams and reverts some of #2836

I've disabled some lints in cli/js/streams/ in order to get this to pass, I've marked them with TODOs,

As well, There are a few other TODO's that i've noted in streams/. notably, we're missing the DomException, and there are some gross type casts. as undefined as ..

There seems to be an impedance mismatch in the streams interface, eg, SDReadbleStream vs ReadableStream, but I'm not sure how much that matters at the moment

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 23, 2019

Should this be a git submodule rather than vendorize?

@nstott

This comment has been minimized.

Copy link
Contributor Author

nstott commented Oct 23, 2019

Should this be a git submodule rather than vendorize?

I was wondering this myself, but there are a bunch of changes that we need to make to the sd-source,
we need to bring in dom_types etc in various places (https://github.com/nstott/deno/blob/enable-sd-streams/cli/js/streams/pipe-to.ts#L17-L18), and we need to add the extension for all the imports (the source repo doesn't have those (https://github.com/stardazed/sd-streams/blob/master/packages/streams/src/pipe-to.ts#L8-L10).

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 23, 2019

Another option is to fork and float a patch, that way we could rebase over future updates...

getWriter(): WritableStreamDefaultWriter<W>;
}

export interface PipeOptions {

This comment has been minimized.

Copy link
@nstott

nstott Oct 23, 2019

Author Contributor

These are called from cli/js/streams, and seem to be expected to exist in the dom.

I pulled most of these interfaces from https://github.com/microsoft/TypeScript/blob/master/lib/lib.dom.d.ts, I'm not sure whether this is right, or the right place to put these

nstott added 2 commits Oct 23, 2019
@nstott nstott force-pushed the nstott:enable-sd-streams branch from 1fc17b3 to 1cc6eb3 Oct 23, 2019
@nstott nstott marked this pull request as ready for review Oct 23, 2019
ry added 2 commits Oct 27, 2019
@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 27, 2019

@nstott I'd like to land this but it's red .. can you look into it?

@nstott

This comment has been minimized.

Copy link
Contributor Author

nstott commented Oct 27, 2019

will do!

…msg.rs and errors.ts
@nstott

This comment has been minimized.

Copy link
Contributor Author

nstott commented Oct 27, 2019

That's done it.
I didn't realize these kinds were shared in both worlds.
I'm not sure this is the right place to put these.
There are a couple of places that sd-streams calls a DomException

https://github.com/stardazed/sd-streams/blob/45aa9d1ab8679cfcc3ca2959f6eef2276197d1c6/packages/streams/src/pipe-to.ts#L38

https://github.com/stardazed/sd-streams/blob/83df410ab4a0d1d049ecd3dccfefc9b36698a701/packages/streams/src/shared-internals.ts#L171

As far as I could tell, we didn't have an equivalent error type, so I shoved these into the DenoError bucket and punted

@nstott

This comment has been minimized.

Copy link
Contributor Author

nstott commented Oct 27, 2019

This is a lot of code that's being imported. If this is too big, I could stage these imports into smaller pieces.

for example, I could rip out the writer and transformer stuff from this pr, as we don't use those features yet, and don't have tests in place yet for that specific functionality.

@ry

This comment has been minimized.

Copy link
Collaborator

ry commented Oct 28, 2019

FYI - this PR adds 1mb to the release executable on mac:

> ls -l target/release*/deno
-rwxr-xr-x  1 rld  staff  45396256 Oct 28 11:14 target/release-streams/deno
-rwxr-xr-x  2 rld  staff  44490880 Oct 28 11:16 target/release/deno
@ry ry referenced this pull request Oct 28, 2019
@nstott nstott force-pushed the nstott:enable-sd-streams branch from afc153a to b75f3fa Oct 28, 2019
@ry
ry approved these changes Oct 28, 2019
Copy link
Collaborator

ry left a comment

LGTM - thank you !

@ry ry merged commit 65d9286 into denoland:master Oct 28, 2019
10 checks passed
10 checks passed
test macOS-10.14
Details
test_std macOS-10.14
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@nstott nstott deleted the nstott:enable-sd-streams branch Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.