Skip to content

Build for Deno#452

Closed
Soremwar wants to merge 11 commits into
nodejs:masterfrom
Soremwar:deno
Closed

Build for Deno#452
Soremwar wants to merge 11 commits into
nodejs:masterfrom
Soremwar:deno

Conversation

@Soremwar

@Soremwar Soremwar commented Nov 17, 2020

Copy link
Copy Markdown

Discussion in #451

This needs some polishing, namely:

  • Enable tests to work based on the Deno build
  • Await some of my PRs to be merged into Deno repo that fix some issues with their current Node polyfill
  • Discuss method of publishing, and talk about mantainability for the future
  • Document Deno polyfills
  • Document Crossplatform variables and modules

@Soremwar

Copy link
Copy Markdown
Author

Anyone that can help me debug this fails? I tried to test them on Node 6.17.1 and they all passed

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The changes done inside lib must be done inside build/ instead. There is a bunch of scripts there that takes code from Node.js core and rewrite it so it's agnostics. We could potentially use some of the same machinery to generate a deno build.

@mcollina

Copy link
Copy Markdown
Member

(Unfortunately I do not have time to debug the build right now, I'm going for some extended leave and I'll be back full time next year).

@mcollina mcollina requested a review from benjamingr November 19, 2020 16:29
@benjamingr

Copy link
Copy Markdown
Member

@mcollina can you point me at the code that generates readable-stream from Node core? I'm not actually familiar with it :]?

@targos

targos commented Nov 19, 2020

Copy link
Copy Markdown
Member

@Soremwar

Soremwar commented Nov 22, 2020

Copy link
Copy Markdown
Author

Someone knows what version of Node the last build of readable stream is using? The README points out to it being Node 10 but I can't really tell (needed for running the build script)

@benjamingr

Copy link
Copy Markdown
Member

Looking at the issue I think it's v10.18.1

@Soremwar

Copy link
Copy Markdown
Author

Thanks @benjamingr

@mcollina

Copy link
Copy Markdown
Member

This seems pretty amazing! Are there any tests for this?

@Soremwar

Soremwar commented Dec 31, 2020

Copy link
Copy Markdown
Author

@mcollina Actually...that's the main reason why there's been so little activity in this since it was created. The build has been working for a while, but making the tests work has proven to be a major headache due to the differences between the Deno test suite and something like tap

More progress to follow though, I'm on vacations so I am putting some effort into landing this

@mcollina

Copy link
Copy Markdown
Member

You can also write a few high level tests. We run separate tests for the browser for this reason, see https://github.com/nodejs/readable-stream/tree/master/test/browser for more details.

@mcollina

mcollina commented Jan 7, 2021

Copy link
Copy Markdown
Member

FYI: check out #454.

@jimmywarting

Copy link
Copy Markdown
Contributor

I doubt that Deno won't get much traction for node streams, they are mostly just focused on web standarder, meaning: whatwg streams. Node also recently got them.

I always try to avoid core builtin's when i develop anything for cross platform that runs in Deno, Browser and Node.
I never want to bundle the hole readable-stream into the browser, it cost too much, I always try to make my page score 100% in lighthouse
i got a own size limit to somewhere like < 100 kb when i develop a js library for browser and node:stream can easily break that threshold

@mhdawson mhdawson deleted the branch nodejs:master September 24, 2021 20:23
@mhdawson mhdawson closed this Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants