2.x.x #61

Merged
merged 5 commits into from Jan 17, 2012

Conversation

Projects
None yet
3 participants
Contributor

disfated commented Jan 8, 2012

Sorry for messed everything up again.
What is done under lots of stuff commit:

  • support for recursive redirect
  • all error-events pass instance of Error as a first param (not a string)
  • some refactorings
  • error event is not emitted on 4xx status codes, as it actually not an error. Instead fail is emitted.
  • rewriten all testcases using nodeunit, few added.
  • updated readme

If you find it worthy to merge, you must probably increase the major version, as it adds some new api (error, fail events, different types passes in callbacks).

Collaborator

ayoung commented Jan 9, 2012

This is awesome. Thanks for all these fixes. I'll have to pull it into the 0.3.x branch. I think we'll have to rename the 0.3.x branch to 2.x.x. We're currently on 0.x.x meaning its still in development. But with 400+ followers of restler, it is hard to believe it isn't already being used in production somewhere. Are you using restler on prod?

Contributor

disfated commented Jan 9, 2012

I'm going to make it work on node 0.4.x. Currently abort method is not working properly because of different ClientRequest behaviour and some exceptions in iconv thrown. After these will be fixed, I think it is safe to pull into master.

Contributor

disfated commented Jan 9, 2012

I wonder if it is of a great pain to stop supporting old node versions?
The internals of ClientRequest in 0.4.x are completely different - there will be if node_version < '0.6.0' ... ugly hacks. Also iconv terminates the process with some core exception. Don't even want to waste my time digging into it...
Is it really worth our time to support old node's, considering 0.6.x is the only stable crossplatform release?

bcherry commented Jan 9, 2012

Heroku currently only officially supports 0.4.7, although it's fairly simple to vendor an upgraded version if you're comfortable hacking build scripts, and @ddollar appears to be working on official support for running any version of node. Not sure about the state of other popular node hosting providers.

Collaborator

ayoung commented Jan 9, 2012

@disfated are you saying the code base as it stands right now is only node 0.6.x compat?

Contributor

disfated commented Jan 10, 2012

Yes. Test only pass on 0.6. Actually we could transparently disable features that fail on 0.4:

  • zlib is not available, and if accept-encoding won't be sent to server, it shouldn't compress responses.
  • abort() will throw an error informing user that it is not supported by node version being run.
  • iconv will not be supported.
    So, old nodes will have all the features they have now.
    Btw, current master version will fail on 0.4.x because of zlib.
Collaborator

ayoung commented Jan 10, 2012

i think it would be good to do it for the 0.2.x versions but when i pull in your changes and bump version to 2.x.x I think it would make sense to just support node 0.6.x.

Contributor

disfated commented Jan 10, 2012

2,x.x would be the best to my mind. We could indicate node >= 6.x in dependencies, so npm won't update restler on old nodes. In that case I see no conflicts. The platform is moving on and we must keep it up :)

Collaborator

ayoung commented Jan 12, 2012

Iconv seems like a pretty big dependency to have. Are you using it to try to detect the encoding if it isn't provided?

Contributor

disfated commented Jan 12, 2012

Conversion is made only when content-encoding: some; charset=some present in headers. No detection made.

Contributor

disfated commented Jan 12, 2012

Actually we could omit emitting error event when iconv fails and give the original response to the user in this case.

Collaborator

ayoung commented Jan 12, 2012

What do you think about adding Iconv as a light dependency? Kind of like how it has a light dependency on the yaml lib. Only use it if the lib is installed. Make it optional to install if needed.

Contributor

disfated commented Jan 12, 2012

This's completely okey :)
I'll try to do it today.
Tell me what else I should do to make you merge this.

Contributor

disfated commented Jan 15, 2012

@ayoung is there something left to do?

Collaborator

ayoung commented Jan 15, 2012

No. This looks good now. I'll merge it in tomorrow. Still need to organize the branches. Sorry for the delay.

Contributor

disfated commented Jan 16, 2012

There's a problem (#63). As I've said before restler 0.2.4 uses zlib which is found only in 0.6.x...
This should be fixed somehow if we want old-nodes to be supported with this version.
Sorry, this is my fault ((

Collaborator

ayoung commented Jan 16, 2012

We should make zlib a dependency for the 0.2.x versions but remove the dependency for restler 2.x.x and only support node 0.6.x. What do you think? I think the fixes in the API for 2.x.x is much better suited for node 0.6.x anyway.

Contributor

disfated commented Jan 16, 2012

Wrong.
If you install 0.2.4 version it will work only with node 0.6.x. npm install zlib won't help, because it is a different library - not compatible with native zlib (also lacks gzipping). In other words it is not "node": ">= 0.3.7" compatible as specified. It was my fault not checking this.
In order to solve this, you probably should remove all the zlib-features from 0.2.4 and bump 0.2.5.

Collaborator

ayoung commented Jan 16, 2012

Oh. I see what's happening now. Thanks for explaining that. If you're able to, would you mind removing zlib from 0.2.4 on the 0.2.x branch (not master) and send a completely separate pull request for it? Once fixed I'll update restler on npm for 0.2.5.

Contributor

disfated commented Jan 16, 2012

Ok. But don't blame me if I'll mess everything up again :)

@ayoung ayoung merged commit 00c4011 into danwrong:master Jan 17, 2012

Collaborator

ayoung commented Jan 17, 2012

Ok. Pulled in all your changes into master branch. One prob though. Running the unit tests gives me three tests that fail. I haven't had the time to look into it but thought you might know right away. Any ideas on what the issue is here?

✖ Charsets - Should correctly convert charset windows-1252
Assertion Message: hashes should match
AssertionError: '144d87998fec41e1d3a1bd8f92307ab7' == 'b9bd334aeb238eb104628168cb011351'

✖ Charsets - Should correctly convert charset gbk
Assertion Message: hashes should match
AssertionError: 'bd620df045adcaa3a45df4784a188798' == '01329db97a6a202ecffaf95d4f77a18d'

✖ Charsets - Should correctly convert charset gb2312
Assertion Message: hashes should match
AssertionError: '523295b46978e86aa237c07da6a11f5e' == 'ab788473ee3b5f5fff5eba4ca6172834'
Contributor

disfated commented Jan 17, 2012

I suppose that git "fixed" whitespaces in sample files while merging or something. Actually, I think these charset tests should be simplified. I'll think about it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment