Chunked `writeString` version working under aff-4.0.0 + related test cases

[paluh]

I've made clean version of this pull request from master. I've merged your commit for chunked write and adapted it to aff-4.0.0 in my next commit, so we have quite clear history log.

I've also provided test cases (one of them were failing without these fixes) with simple writable Stream implementation... Unfortunately I'm not able to reproduce empty string problem, so I have to ensure if it is still an issue.

Regarding coding style, I've noticed that unicode is mixed with ASCII versions of some language constructs. For example in Hyper.Node.Server you can find forall and ∀. I have used unicode in my commits, but I don't have any strong preference and I can change that.... Maybe we should stick to one coding style and add some note to the "contributing" section in README? Of course I can help adapt current state of hyper code base.

[paluh]

I just noticed that in my commit coding style is mixed too ;-)

[paluh]

I think that write function from Hyper.Node.Server should also be chunked...

[owickstrom]

Great job! I've run the tests and looked at the code. Only some warnings of unused imports in the Hyper.Node.Server module left, if you'd like to fix those? 👍

Regarding style, I think I'd prefer ASCII. No big deal though. If you'd like to change your stuff that is nice, otherwise you can leave it. I've probably committed with different styles all over the place.

[owickstrom]

I agree regarding write, it should probably use the same chunked write. Maybe it could work on buffers instead, and we can use Node.Buffer.fromString in the writeString version?

[paluh]

Only some warnings of unused imports in the Hyper.Node.Server module left, if you'd like to fix those?

I've left those fixes to separate commit (to ease code review) and finally forgotten to make this cleanup ;-)

Maybe it could work on buffers instead, and we can use Node.Buffer.fromString in the writeString version?

Yeah, I'm going to check if that is feasible soon ;-)

[paluh]

I've got somewhat good news... it seems that this whole tango with chunking was completely unnecessary. Not to mention my test "buffers" etc. On my machine I'm able serve up to 250 Mb in a single chunk with simple write call and save it successfully with wget on disk. This 250 MB limit is just limit on string length on my node instance and not any node streaming restriction.

I've started to test buffer write scenario because I wanted to be sure that I'm really fixing it too... I wasn't able to easily reproduce any problematic behavior. I've reduced my test case and fallen back to really basic javascript server:

https://gist.github.com/paluh/34eb571bd02ac60cf7af8117bfbb0081

Like I've just said - I'm able to use up to 250 MB chunk size and serve it at once. I'm just not able to create larger string (I'm getting RangeError: Invalid string length on string concatenation). Maybe tommorow I will check how large files I'm able to serve with it. It seems that the whole problem was the check of the write return value in writeString function. In other words current write implementation seems to be completely correct - it doesn't chop anything and waits to the end of writing. I think we should make similar one for writeString.

Of course if we want to serve really large files we have to provide some streaming mechanisms... but to be honest - serving large files from node/hyper is not on my priority list ;-)

Tommorow I will test everything again and probably provide another pull request...

[paluh]

Current version of my pull request is a huge simplification and it seems that it fixes this problem. I've tested concurrent fetch (using 10 instances of wget ;-)) from this simple app which serves more then 100 MB without problems:

https://gist.github.com/paluh/b3343ccf64ab82ed7d820b11ed5d2687

[paluh]

I'm not entirely sure why but it seems that travis runs test which is no longer on my branch. There is no such test spec as Hyper.Node.ServerSpec. I've overwritten these branches/pull requests many times, so everybody can be confused. Even our old good Travis ;-)

[paluh]

I've cleared travis cache for this one too and it passes now.

[owickstrom]

@paluh All right! Haven't checked fully yet, but guessing the generated CommonJS modules are cached, and found by the test runner:

Cache: https://github.com/owickstrom/hyper/blob/master/.travis.yml#L70 Test glob: https://github.com/owickstrom/hyper/blob/master/test/Main.purs#L11

We should perhaps not cache the output directory at all.

[owickstrom]

To remind me, does this make #38 obsolete? :)

[paluh]

@owickstrom Yeah, it makes #38 obsolete. Can I close #38 and merge this PR?

[owickstrom]

@paluh Yes, thanks!