Skip to content

Integrate node-streams-aff into library #52

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 10 commits into from
Jul 8, 2023

Conversation

JordanMartinez
Copy link
Contributor

Description of the change

Copies node-streams-aff and its tests into this library, excluding two APIs: newReadable and push. I think that function could be better supported to allow for custom streams, but I didn't port that or implement my own version for two reasons:

  1. I didn't want to think through what that would be yet.
  2. Per the tests, it seemed like it was only used to produce a Readable from a String, which has been supported via fromString.

@jamesdbrock, can you take a look at this?


Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0000)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@JordanMartinez
Copy link
Contributor Author

I'll merge this for now, but if there are any other changes we should make, we can do it in a future PR.

@JordanMartinez JordanMartinez merged commit c6e0134 into master Jul 8, 2023
@JordanMartinez JordanMartinez deleted the integrate-streams-aff branch July 8, 2023 15:38
@jamesdbrock
Copy link
Member

I'll merge this for now, but if there are any other changes we should make, we can do it in a future PR.

Great, thanks. I'll check this out later.

@JordanMartinez
Copy link
Contributor Author

@jamesdbrock Have you had a chance to review this? Since this repo hasn't had a new release yet, it's blocking the other work I've done.

@jamesdbrock
Copy link
Member

Yeah looks great. I see you've swapped in all the new EventEmitter stuff and you've got all the tests passing.

@jamesdbrock
Copy link
Member

The foreign imports in the tests is weird though, why was that necessary? Can't those foreign imports be imported instead from other purescript-node packages?

@JordanMartinez
Copy link
Contributor Author

The foreign imports in the tests is weird though, why was that necessary? Can't those foreign imports be imported instead from other purescript-node packages?

Unfortunately, no, because all of those depend on node-streams.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants