Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Updates for latest version of Journey #16

Closed
wants to merge 1 commit into from

Conversation

justmoon
Copy link

@justmoon justmoon commented Apr 3, 2011

The journey library apparently mixed up its API quite a bit since the last release of webservice.js. This pull request contains the changes I needed to get it to work.

@Marak
Copy link
Owner

Marak commented Apr 7, 2011

Could you help clarify a few points?

I'm ready to update to the new Journey APIs, but it seems all the tests for webservice.js are still passing here when using Journey 0.4.0-pre-2. Are you seeing the same behavior? I might be messing up here. Either way, I'm thinking we might need to add a few more tests if you were seeing failures but the tests were reporting all passing, I'd like to keep the coverage for this library high.

@Marak
Copy link
Owner

Marak commented Apr 7, 2011

Ohh, and thank you for the patch! :-)

@justmoon
Copy link
Author

justmoon commented Apr 8, 2011

I didn't run the tests, I only tried to create a simple webservice using the library. Among other things it triggered this error from Journey:

if (arguments[0].headers) { throw new(Error)("Router#route method renamed to 'handle'") }

The changes in this pull request are only what I needed to get the basic functionality running at all. The Journey version I used was also 0.4.0-pre-2. After this pull request I spent another hour bugfixing, eventually gave up and wrote my own webservice generator.

I honestly don't know if I was doing something totally wrong (probably). :)

@Marak
Copy link
Owner

Marak commented Apr 8, 2011

Well, best of luck with that!

In the future, you should probably pay attention to which versions you are using and what tests are passing.

Current package.json specificies the Journey version @ 0.2.8
https://github.com/Marak/webservice.js/blob/master/package.json#L14

So I'm sorry you wasted your time trying to use the wrong version and not running any of the tests.

@Marak Marak closed this Apr 8, 2011
@justmoon
Copy link
Author

justmoon commented Apr 9, 2011

Relax, I'm not trying to diminish your work or anything. If Journey breaks downward compatibility in their library that's their problem not yours. I still really like webservice.js and am using it in several other projects, it's just not right for this particular one. (The journey 0.4.0 requirement isn't the only reason, the client also wanted a domain-specific language for the modules rather than plain CommonJS modules.)

Regarding the package.json, it currently specifies 0.4.0-pre-2 (because of the ">="). To have npm use 0.2.8 instead, specify "= 0.2.8" or a range like ">= 0.2.8 < 0.4.0".

I ran the tests against both master and my branch:

moon@clymene:/var/www/webservice.js$ vows tests/*

/usr/local/lib/node/.npm/journey/0.4.0-pre-2/package/lib/journey.js:82
        if (arguments[0].headers) { throw new(Error)("Router#route method rena
                                          ^
Error: Router#route method renamed to 'handle'
    at Object.route (/usr/local/lib/node/.npm/journey/0.4.0-pre-2/package/lib/journey.js:82:43)
    at /atlas/www/webservice.js/lib/createHandler.js:8:12
    at IncomingMessage.<anonymous> (/atlas/www/webservice.js/lib/createServer.js:15:11)
    at IncomingMessage.emit (events.js:39:17)
    at HTTPParser.onMessageComplete (http.js:111:23)
    at Socket.ondata (http.js:980:22)
    at Socket._onReadable (net.js:654:27)
    at IOWatcher.onReadable [as callback] (net.js:156:10)
moon@clymene:/var/www/webservice.js$ git checkout journeyfixes
Switched to branch 'journeyfixes'
moon@clymene:/var/www/webservice.js$ vows tests/*
·············??····· 

  When using webservice with test configuration a request against /
    ? should respond with 200
      » expected 200,
    got  500 (==) // test-webservice.js:27

    ? and should return html view representing web-service
      » expected '<',
    got  '{' (==) // test-webservice.js:30

? Broken » 18 honored · 2 broken (2.010s)

So current master crashes (for me) and my branch fails two tests. I'll take a look if I can fix the remaining two. Sorry for not running the tests before submitting the pull request.

@justmoon justmoon reopened this Apr 9, 2011
@justmoon
Copy link
Author

justmoon commented Apr 9, 2011

The test suite now runs cleanly:

moon@clymene:/var/www/webservice.js$ vows tests/*
···················· 

? OK » 20 honored (2.008s)

Unfortunately github didn't update the pull request with the new commit - perhaps because it was closed when I committed it? In any case here it is, it's a one-liner:

justmoon@df8d95d

@justmoon justmoon closed this Apr 9, 2011
@Marak
Copy link
Owner

Marak commented Apr 10, 2011

I'm totally retarded. I was operating in the 0.5.0 branch, not master.

I totally forgot I had fixed all this already in https://github.com/Marak/webservice.js/commits/v0.5.0

Sorry for the confusion, I'm merging everything into master now.

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

Successfully merging this pull request may close these issues.

None yet

2 participants