search

intesso / connect-livereload

connect middleware for adding the livereload script to the response
MIT License
305 stars 53 forks source link

Deal appropriately with buffered responses when chunks are arbitrarily small. #36

Closed drawveloper closed 10 years ago

drawveloper commented 10 years ago

As described on issue #35, buffered responses with small chunks were not properly detected.

drawveloper commented 10 years ago

It seems your travis config is not well setup. Every job on node 0.9 fails. However, all tests pass on the executing node versions.

andineck commented 10 years ago

yes, I noticed this too. I love travis but with older versions of node it's struggling. First I had to get rid of 0.6 then 0.8 and now 0.9 is starting to cause problems...

andineck commented 10 years ago

Would you mind sending a PR for just the test case?

drawveloper commented 10 years ago

But this PR would break the tests.

drawveloper commented 10 years ago

You can checkout this PR as a branch:

git fetch origin refs/pull/36/head:pr/36
git checkout pr/36

And if you want to go the test commit:

git reset --hard 280563f
drawveloper commented 10 years ago

Can you accept this pull request? Live reload is actually broken in many cases where the response is buffered, without it.

drawveloper commented 10 years ago

I just added a small commit to allow images with a querystring on the URL to also be ignored.

drawveloper commented 10 years ago

@andi-neck what do you think about adding me as a contributor? I could manage the PR's better that way.

andineck commented 10 years ago

@gadr I just added you as contributor. Thank you for the work you already put into this module and thank you for the effort to keep improving this module. Please test your changes also against corrupting non html files #39 .

drawveloper commented 10 years ago

thanks, @andi-neck. I'm in vacations right now, but i'll get to it if I have some time on the PC :)

On Sat, Jun 14, 2014 at 5:28 PM, andi-neck notifications@github.com wrote:

@gadr https://github.com/gadr I just added you as contributor. Thank you for the work you already put into this module and thank you for the effort to keep improving this module. Please test your changes also against corrupting non html files #39 https://github.com/intesso/connect-livereload/issues/39 .

— Reply to this email directly or view it on GitHub https://github.com/intesso/connect-livereload/pull/36#issuecomment-46098752 .

drawveloper commented 10 years ago

Hey, @andi-neck. I'm sorry I didn't quite got around to finishing this. What would you like me to do in order to merge? I'll rebase it right now with master.

drawveloper commented 10 years ago

Hey, @andi-neck, done. It's on top of master now. As for #39, this pr would probably fix that as we now wait for the whole response to check if/where we should add the tag. This was the problem with chuncked responses: it would fail to detect where to inject the tag and (a) not inject it anywhere or (b) inject it somewhere invalid.

Can we merge and release this as a new minor?

andineck commented 10 years ago

@gadr thx. I just released v0.4.1 before merging this to have an up to date published version before this PR. I would now also increase the minor to v0.5.0.

andineck commented 10 years ago

I just checked the code. It looks good as far as I can see from the code. Do you want me to merge it? Concerning #39 I agree that we should re check after this merge.

drawveloper commented 10 years ago

Thank you. Yes, if you could merge and publish, that would be lovely. I have some projects needing to be freed from my github URL :)

andineck commented 10 years ago

done

drawveloper commented 10 years ago

Thanks!