Remove the last parameter to Stream

[hdgarrood]

Not ready for merging. I'm not yet sure that this is the right approach, see #3.

I guess we should be asking: how likely is it that a PureScript user will want to get readable streams from JavaScript where the encoding is already set, and do stuff with them in PureScript? As opposed to getting them directly in PureScript. For example, I think we can safely assume that PureScript bindings to http or fs.createReadStream won't set an encoding. And: is it acceptable to say that, in the case where a readable stream with an encoding somehow finds its way into a PureScript program, people will need to use some other API?

If we do stick with the approach that these commits currently use, which is deciding that this API only works with readable streams without an encoding set, we should remove setEncoding and related tests. I haven't gotten around to that just yet.

[hdgarrood]

I'm happy with this now. It's sort of halfway between the two options I suggested earlier. There's a new onDataEither function, which is useful in the case where you're not sure if setEncoding has been called. setEncoding has documentation discouraging its use - I think the only use case is if you're interoperating with JS that expects it to have been called, for some reason. onData and onDataString both assume that setEncoding hasn't been called (and throw if it has). onDataString just decodes the chunk using the given encoding before passing it to the callback.

[paf31]

Does it make sense to put this in a node-process library like we have HTTP streams in node-http?

I'm wondering if process objects can be obtained in other ways than just require('process').

[hdgarrood]

Quite possibly, yes. I was thinking that library (once it exists) could re-export these.

I'm fairly sure that the only way of getting a process object is the global process variable (not require('process')). The only other similar thing I can think of is a child process, but they are different; for instance, a child process' stdin, stdout, or stderr property might be null.

[hdgarrood]

Actually, on second thoughts, yes, I think a node-process library is a better fit for these. Shall I remove these, and then I can create that library once this is PR merged and released?

[paf31]

I think that makes sense. Once the API is nailed down, things like gzip and crypto streams would probably make nice libraries too.

[hdgarrood]

👍 Ok, done. Agree re gzip and crypto. I think they would be pretty straightforward to wrap too, which is nice.

[paf31]

Looks great! Thanks very much for this.

[paf31]

I like the new approach to encodings as well. I think this should work fine for node-http, but I guess we'll see when this is merged.

I've been wondering lately if it makes sense to just create a monolithic purescript-node package, for all of the low-level bindings, to try to deal with this stuff all together.

[hdgarrood]

:)

I don't feel particularly strongly either way about a monolithic repo. The only drawback I can think of is losing the granularity - you might have to upgrade all your code that deals with the filesystem just because you needed a bug fix in the http module, or something.

I agree that it is a bit annoying at the moment, having to change a bunch of different libraries because of changes like this, but I also think these sorts of changes will quickly become less frequent.

[hdgarrood]

Oh, also - this fixes #3.

(edit: huh, I was expecting that to mark #3 so that it gets closed automatically after this is merged. Maybe that only works if you haven't referenced it already in the same PR.)

[paf31]

Typo?

[hdgarrood]

oops, not sure what happened there.

[hdgarrood]

Ok, I think we're there now