Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-2350

Add async calls to normal JavaScript

Details

    • Patch Available

    Description

      Currently async call are only available with -gen js if you unwire the client send/recv methods (pretty messy and unintuitive) or if you use jQuery (-gen js:jquery). This patch makes it easy to use async callbacks with any Thrift call by simply appending the desired callback to the args). e.g.

      client.myFunc(arg1, arg2, function (result) {
      //do callback stuff with result
      //result will be the normal return value or the exception
      });

      This patch preserves the existing sync call style (just leave off the callback).

      Compiler js generator changes
      =============================

      • Combining node and jquery switches now results in a compile error rather than silently producing corrupt code in ./gen-nodejs
      • Updated comments
      • Added ability to make async calls without jQuery

      Node test client.js
      ===================

      • Corrected comments and spelling

      JS thrift.js
      ====================

      • Added support for async calls w/o jQuery

      Node/JS test.js
      ====================

      • Updated comments to clarify jQuery dependency (-gen js:jquery)

      Node/JS test-nojq.js test-nojq.html
      ===================================

      • Full test suite for normal JS build (-gen js)

      Node http test server and handler
      =================================
      linted

      Attachments

        Activity

          Better documentation for the library readme and the website is still necessary but a separate ticket can be used for that.

          henrique Henrique Mendonca added a comment - Better documentation for the library readme and the website is still necessary but a separate ticket can be used for that.
          jfarrell Jake Farrell added a comment -

          hey codesf, anything left on this ticket or can it be closed?

          jfarrell Jake Farrell added a comment - hey codesf , anything left on this ticket or can it be closed?
          hudson Hudson added a comment -

          FAILURE: Integrated in Thrift #1040 (See https://builds.apache.org/job/Thrift/1040/)
          THRIFT-2350 Add async calls to normal JavaScript (henrique: rev 2f51f327e3b6d22e578a84a037d1a9094c64dd9b)

          • lib/nodejs/test/test.js
          • lib/nodejs/test/server_http.js
          • lib/nodejs/test/test.html
          • lib/js/test/server_http.js
          • lib/js/test/test_handler.js
          hudson Hudson added a comment - FAILURE: Integrated in Thrift #1040 (See https://builds.apache.org/job/Thrift/1040/ ) THRIFT-2350 Add async calls to normal JavaScript (henrique: rev 2f51f327e3b6d22e578a84a037d1a9094c64dd9b) lib/nodejs/test/test.js lib/nodejs/test/server_http.js lib/nodejs/test/test.html lib/js/test/server_http.js lib/js/test/test_handler.js

          For the CI, Jenkins just builds the whole think as described in the job: https://builds.apache.org/job/Thrift/
          No many people have access to change these setting but it can also be done.
          However, setting up Travis CI is really simple, just fork apache/thrift on github and follow the first two steps from here: http://docs.travis-ci.com/user/getting-started/
          It'll build automatically as soon as any change is made to your fork/repo.

          henrique Henrique Mendonca added a comment - For the CI, Jenkins just builds the whole think as described in the job: https://builds.apache.org/job/Thrift/ No many people have access to change these setting but it can also be done. However, setting up Travis CI is really simple, just fork apache/thrift on github and follow the first two steps from here: http://docs.travis-ci.com/user/getting-started/ It'll build automatically as soon as any change is made to your fork/repo.

          Hey Henrique,

          You are absolutely right regarding the README. Also a few more things need to be added on the grunt front to get the old test running there (the Gruntfile only runs the new JS to Node test presently). I will help out in any way you need, though the CI stuff you guys have setup is opaque to me. When you are good with the config I will also be glad to update the README.

          Best,
          Randy

          codesf Randy Abernethy added a comment - Hey Henrique, You are absolutely right regarding the README. Also a few more things need to be added on the grunt front to get the old test running there (the Gruntfile only runs the new JS to Node test presently). I will help out in any way you need, though the CI stuff you guys have setup is opaque to me. When you are good with the config I will also be glad to update the README. Best, Randy

          You have a point, I'll try to get this running on Travis CI. My worry was just this wouldn't get tested. Perhaps we should have this all documented in the README as it's actually part of the building and deploying process.

          henrique Henrique Mendonca added a comment - You have a point, I'll try to get this running on Travis CI. My worry was just this wouldn't get tested. Perhaps we should have this all documented in the README as it's actually part of the building and deploying process.

          Hi Henrique,

          Thanks for the commit!

          In regards to keeping Node.js source out of the lib/js/test directory, I think I disagree. Presently the JS Java test server is housed there lib/js/test/src/test/Httpd.java). Its role is not to support Java but to support JavaScript, so the location makes some sense. That said I would rather see this Java server moved to the integration test suit because it is really a cross language test not a JavaScript test.

          In the context of Apache Thrift, Browser based JavaScript is half a language, because you can only build clients a server language is needed. If the choice is between Java and Node.js, I think we get many benefits by using Node.js. Node.js is the defacto server side JavaScript language, uses much of the same Apache Thrift compiler code and has many library commonalities. Almost all of the build and project management tools for front end JS are also written in Node.js so it will likely already be in place for browser devs.

          With the server_http.js and test_handler.js in lib/js/test you can go to the lib/js directory, type "npm install" and then "grunt" and the system will lint, concat, min, gen docs and test the JS source using phantom against the Node.js server_http.js. I think this will be straight forward for JS developers, unlike building a Java server. Grunt is the natural counter part to ant/maven/make used elsewhere in the thrift build. At some point I think make should simply drive grunt in the lib/js tree much as it does with ant in lib/java. With the new compiler-only switch I can clone the master, make just the compiler and grunt lib/js and I am ready to work with JavaScript.

          Another thought is that the server_http.js may grow to be more front end test focused over time (Web Sockets, etc), making it less of a fit on the node test side. Also if we include the server_http.js and test_handler.js in lib/js/test and we point Bower at the lib/js path (as Roger suggested) rather than lib/js/src or lib/js/dist, all you need to do to pull a full JS environment into your project is "bower install thrift". You can then "npm install" and "grunt" to build the project (if you have a thrift compiler) with a complete running client/server test, which is very handy as a tutorial. These files are all small.

          I would also say that leaving server_http.js in lib/nodejs/test is fine but if so we need to create a nodejs client to actually test with it. It has no real purpose in the nodejs tree right now and is not wired into testAll.sh or any other test.

          If there is support for creating the described Node JavaScript server in the JavaScript lib/js/test dir, I can add a patch that creates (or moves) lib/js/test/server_http.js and lib/js/test/test_handler.js. If not I need to add a patch to fix the grunt build, it is broken now because of the lib/js/test/server_http.js dependency.

          Interested to hear other views!

          Best,
          Randy

          codesf Randy Abernethy added a comment - Hi Henrique, Thanks for the commit! In regards to keeping Node.js source out of the lib/js/test directory, I think I disagree. Presently the JS Java test server is housed there lib/js/test/src/test/Httpd.java). Its role is not to support Java but to support JavaScript, so the location makes some sense. That said I would rather see this Java server moved to the integration test suit because it is really a cross language test not a JavaScript test. In the context of Apache Thrift, Browser based JavaScript is half a language, because you can only build clients a server language is needed. If the choice is between Java and Node.js, I think we get many benefits by using Node.js. Node.js is the defacto server side JavaScript language, uses much of the same Apache Thrift compiler code and has many library commonalities. Almost all of the build and project management tools for front end JS are also written in Node.js so it will likely already be in place for browser devs. With the server_http.js and test_handler.js in lib/js/test you can go to the lib/js directory, type "npm install" and then "grunt" and the system will lint, concat, min, gen docs and test the JS source using phantom against the Node.js server_http.js. I think this will be straight forward for JS developers, unlike building a Java server. Grunt is the natural counter part to ant/maven/make used elsewhere in the thrift build. At some point I think make should simply drive grunt in the lib/js tree much as it does with ant in lib/java. With the new compiler-only switch I can clone the master, make just the compiler and grunt lib/js and I am ready to work with JavaScript. Another thought is that the server_http.js may grow to be more front end test focused over time (Web Sockets, etc), making it less of a fit on the node test side. Also if we include the server_http.js and test_handler.js in lib/js/test and we point Bower at the lib/js path (as Roger suggested) rather than lib/js/src or lib/js/dist, all you need to do to pull a full JS environment into your project is "bower install thrift". You can then "npm install" and "grunt" to build the project (if you have a thrift compiler) with a complete running client/server test, which is very handy as a tutorial. These files are all small. I would also say that leaving server_http.js in lib/nodejs/test is fine but if so we need to create a nodejs client to actually test with it. It has no real purpose in the nodejs tree right now and is not wired into testAll.sh or any other test. If there is support for creating the described Node JavaScript server in the JavaScript lib/js/test dir, I can add a patch that creates (or moves) lib/js/test/server_http.js and lib/js/test/test_handler.js. If not I need to add a patch to fix the grunt build, it is broken now because of the lib/js/test/server_http.js dependency. Interested to hear other views! Best, Randy
          hudson Hudson added a comment -

          SUCCESS: Integrated in Thrift #1038 (See https://builds.apache.org/job/Thrift/1038/)
          THRIFT-2350 Add async calls to normal JavaScript (henrique: rev a2de4105317adeb5268e5e289a6226d6477cfbfe)

          • lib/js/test/test.html
          • compiler/cpp/src/generate/t_js_generator.cc
          • lib/js/package.json
          • lib/js/src/thrift.js
          • lib/js/Gruntfile.js
          • lib/js/test/test-nojq.html
          • lib/nodejs/test/test_handler.js
          • lib/js/test/test.js
          • lib/js/test/test-jq.js
          • lib/js/README
          • lib/nodejs/test/client.js
          • lib/js/test/test-nojq.js
          hudson Hudson added a comment - SUCCESS: Integrated in Thrift #1038 (See https://builds.apache.org/job/Thrift/1038/ ) THRIFT-2350 Add async calls to normal JavaScript (henrique: rev a2de4105317adeb5268e5e289a6226d6477cfbfe) lib/js/test/test.html compiler/cpp/src/generate/t_js_generator.cc lib/js/package.json lib/js/src/thrift.js lib/js/Gruntfile.js lib/js/test/test-nojq.html lib/nodejs/test/test_handler.js lib/js/test/test.js lib/js/test/test-jq.js lib/js/README lib/nodejs/test/client.js lib/js/test/test-nojq.js
          henrique Henrique Mendonca added a comment - - edited

          Hi Randy,

          I'm committing this right now but I'm not sure about the move from lib/nodejs/test/server_http.js to lib/js/test/server_http.js
          I think it would be nice to keep node and js separated by it's script flavour so I'm committing without it for now.
          It seems that another patch also removed the nodejs unit-tests from "make check" but server_http would be also a good candidate for that make target.
          Thanks again for for another great contribution!

          Cheers,
          Henrique

          henrique Henrique Mendonca added a comment - - edited Hi Randy, I'm committing this right now but I'm not sure about the move from lib/nodejs/test/server_http.js to lib/js/test/server_http.js I think it would be nice to keep node and js separated by it's script flavour so I'm committing without it for now. It seems that another patch also removed the nodejs unit-tests from "make check" but server_http would be also a good candidate for that make target. Thanks again for for another great contribution! Cheers, Henrique

          Hi Henrique,

          Let me know what you think about this arrangement.

          This patch applies over the prior two. It removes all of the redundant JavaScript browser tests from lib/nodejs/test. The nodejs/test dir installs with npm, so we lose these as examples in the dist.

          On the lib/js side the patch breaks browser based JavaScript tests into three mutually exclusive files:
          ++ test.js these are generic tests that should complete with Normal (-gen js) and with jQuery (-gen js:jquery) builds.
          ++ test-jq.js these are tests that only work with the jQuery build (-ge js:jquery)
          ++ test-nojq.js these are tests that only work with Normal builds (-gen js)

          The test.html works as before, testing the jQuery (-gen js:jquery) implementation, now pulling tests from test.js and test-jq.js. The new test-nojq.html tests the normal (-gen js) build and runs all of the tests in test.js and test-nojq.js. At present build.xml only runs test.html and grunt only runs test-nojq.html. More to do...

          codesf Randy Abernethy added a comment - Hi Henrique, Let me know what you think about this arrangement. This patch applies over the prior two. It removes all of the redundant JavaScript browser tests from lib/nodejs/test. The nodejs/test dir installs with npm, so we lose these as examples in the dist. On the lib/js side the patch breaks browser based JavaScript tests into three mutually exclusive files: ++ test.js these are generic tests that should complete with Normal (-gen js) and with jQuery (-gen js:jquery) builds. ++ test-jq.js these are tests that only work with the jQuery build (-ge js:jquery) ++ test-nojq.js these are tests that only work with Normal builds (-gen js) The test.html works as before, testing the jQuery (-gen js:jquery) implementation, now pulling tests from test.js and test-jq.js. The new test-nojq.html tests the normal (-gen js) build and runs all of the tests in test.js and test-nojq.js. At present build.xml only runs test.html and grunt only runs test-nojq.html. More to do...

          Hi Henrique,

          The lib/js/test/test-nojq.js test is independent of jQuery, unlike test.js, and allows you to run tests without the jQuery dependency. This allows us to test thrift -gen js. Right now the tests only exercise the thrift -gen js:jquery branch of the compiler. I'll try to reog the test.js and test-nojq.js files to extract the commonalities and repost the patch asap.

          Best,
          Randy

          codesf Randy Abernethy added a comment - Hi Henrique, The lib/js/test/test-nojq.js test is independent of jQuery, unlike test.js, and allows you to run tests without the jQuery dependency. This allows us to test thrift -gen js. Right now the tests only exercise the thrift -gen js:jquery branch of the compiler. I'll try to reog the test.js and test-nojq.js files to extract the commonalities and repost the patch asap. Best, Randy

          Hi Randy,

          Thank you for the patch! This is really nice and we should have it in this next release, but would you be able to have a single unified test instead of copying the whole test file again?
          You can check the return object on the async calls, or use a URL parameter, but test-nojq.js is not really necessary in my opinion. What do you think?
          Thanks again for the good work!

          Cheers,
          Henrique

          henrique Henrique Mendonca added a comment - Hi Randy, Thank you for the patch! This is really nice and we should have it in this next release, but would you be able to have a single unified test instead of copying the whole test file again? You can check the return object on the async calls, or use a URL parameter, but test-nojq.js is not really necessary in my opinion. What do you think? Thanks again for the good work! Cheers, Henrique

          JavaScript test updates for prior async additions (requires 0001 patch). Gruntfile has be improved so that all dependencies are resolved and phantom tests succeed independent of make.

          TODO:

          • grunt only tests test-nojq.html against node server, need to add the old jQuery based test.html
          • When above is done would be nice to have make use grunt for js build and test
          codesf Randy Abernethy added a comment - JavaScript test updates for prior async additions (requires 0001 patch). Gruntfile has be improved so that all dependencies are resolved and phantom tests succeed independent of make. TODO: grunt only tests test-nojq.html against node server, need to add the old jQuery based test.html When above is done would be nice to have make use grunt for js build and test

          corrected patch

          codesf Randy Abernethy added a comment - corrected patch

          patch

          codesf Randy Abernethy added a comment - patch

          People

            henrique Henrique Mendonca
            codesf Randy Abernethy
            Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved: