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

Build for Deno #452

Closed
wants to merge 11 commits into from
Closed

Build for Deno #452

wants to merge 11 commits into from

Conversation

Soremwar
Copy link

@Soremwar Soremwar commented Nov 17, 2020

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
Author

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

Copy link
Member

@mcollina mcollina left a comment

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

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

@targos
Copy link
Member

targos commented Nov 19, 2020

@Soremwar
Copy link
Author

Soremwar commented Nov 22, 2020

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
Member

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

@Soremwar
Copy link
Author

Thanks @benjamingr

@mcollina
Copy link
Member

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

@Soremwar
Copy link
Author

Soremwar commented Dec 31, 2020

@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
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
Copy link
Member

mcollina commented Jan 7, 2021

FYI: check out #454.

@jimmywarting
Copy link
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