Details

    • Patch Info:
      Patch Available

      Description

      I have implemented BinaryProtocol for the JS library. Implementation provided with unit tests. Looking for feedback.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user radekg opened a pull request:

          https://github.com/apache/thrift/pull/345

          THRIFT-2926: JS: Binary protocol with unit tests.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/radekg/thrift js-binary-protocol

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/thrift/pull/345.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #345


          commit 14f0f6b1a3989af01fea1a1de44f5d602665e719
          Author: radekg <radek@gruchalski.com>
          Date: 2015-01-02T16:12:45Z

          JS: Binary protocol with unit tests.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user radekg opened a pull request: https://github.com/apache/thrift/pull/345 THRIFT-2926 : JS: Binary protocol with unit tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/radekg/thrift js-binary-protocol Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/345.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #345 commit 14f0f6b1a3989af01fea1a1de44f5d602665e719 Author: radekg <radek@gruchalski.com> Date: 2015-01-02T16:12:45Z JS: Binary protocol with unit tests.
          Hide
          radekg Radoslaw Gruchalski added a comment -
          Show
          radekg Radoslaw Gruchalski added a comment - Pull request at: https://github.com/apache/thrift/pull/345
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user radekg commented on the pull request:

          https://github.com/apache/thrift/pull/345#issuecomment-68580492

          This build has failed because of some jslint issues. Any hints how can I help resolving?

          Show
          githubbot ASF GitHub Bot added a comment - Github user radekg commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-68580492 This build has failed because of some jslint issues. Any hints how can I help resolving?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bufferoverflow commented on the pull request:

          https://github.com/apache/thrift/pull/345#issuecomment-68587476

          jslint is used via lib/js/test/build.xml which also runs the unit tests test.js/test.html

          My favorite would be integration of your tests into test.js/test.html and use guessProtocolFactory within the server part https://github.com/apache/thrift/blob/master/lib/js/test/src/test/Httpd.java#L217

          Show
          githubbot ASF GitHub Bot added a comment - Github user bufferoverflow commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-68587476 jslint is used via lib/js/test/build.xml which also runs the unit tests test.js/test.html My favorite would be integration of your tests into test.js/test.html and use guessProtocolFactory within the server part https://github.com/apache/thrift/blob/master/lib/js/test/src/test/Httpd.java#L217
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user radekg commented on the pull request:

          https://github.com/apache/thrift/pull/345#issuecomment-68589507

          Thank you. I'll take a look at this.

          Show
          githubbot ASF GitHub Bot added a comment - Github user radekg commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-68589507 Thank you. I'll take a look at this.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user radekg commented on the pull request:

          https://github.com/apache/thrift/pull/345#issuecomment-68693024

          @bufferoverflow I am trying to understand where the problem is. It seems that travis is using a different method of testing than the one documented in the README for js tests. The README assumes grunt as a main way of testing while travis seems to be using ant?
          I am also trying to figure out how does one run the main set of tests. Do I have to follow this write up https://thrift.apache.org/docs/BuildingFromSource ?
          Regarding the failing tests, travis complains about some invalid bitwise operations while bitwise is allowed by build.xml jslint. Not sure where to start looking for stuff.

          Show
          githubbot ASF GitHub Bot added a comment - Github user radekg commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-68693024 @bufferoverflow I am trying to understand where the problem is. It seems that travis is using a different method of testing than the one documented in the README for js tests. The README assumes grunt as a main way of testing while travis seems to be using ant? I am also trying to figure out how does one run the main set of tests. Do I have to follow this write up https://thrift.apache.org/docs/BuildingFromSource ? Regarding the failing tests, travis complains about some invalid bitwise operations while bitwise is allowed by build.xml jslint. Not sure where to start looking for stuff.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user radekg commented on the pull request:

          https://github.com/apache/thrift/pull/345#issuecomment-68758030

          I've done a little bit more research into this. There's a major issue which will surface in the unit test once I can get it integrated. I've got the test properly integrated in the grunt task. What happens it that PhantomJS doesn't like the following expression:

          String.fromCharCode.apply(null, new Uint8Array( buffer ) )

          This has been described here: https://github.com/mozilla/pdf.js/issues/1955
          The problem is, if I apply the "fix" mentioned in the comments, the test doesn't pass because unicode characters are decoded incorrectly. So, one way or another, the test will fail. I can see only one solution to this issue. I am going to close this pull request and provide the ThrifBinaryProtocol for JS as an external module to be added on top of Thrift library. Once this is done, I will post a link to the project here.

          Show
          githubbot ASF GitHub Bot added a comment - Github user radekg commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-68758030 I've done a little bit more research into this. There's a major issue which will surface in the unit test once I can get it integrated. I've got the test properly integrated in the grunt task. What happens it that PhantomJS doesn't like the following expression: String.fromCharCode.apply(null, new Uint8Array( buffer ) ) This has been described here: https://github.com/mozilla/pdf.js/issues/1955 The problem is, if I apply the "fix" mentioned in the comments, the test doesn't pass because unicode characters are decoded incorrectly. So, one way or another, the test will fail. I can see only one solution to this issue. I am going to close this pull request and provide the ThrifBinaryProtocol for JS as an external module to be added on top of Thrift library. Once this is done, I will post a link to the project here.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user radekg closed the pull request at:

          https://github.com/apache/thrift/pull/345

          Show
          githubbot ASF GitHub Bot added a comment - Github user radekg closed the pull request at: https://github.com/apache/thrift/pull/345
          Hide
          codesf Randy Abernethy added a comment -

          The JavaScript test suite is not perfectly aligned with the current Apache Thrift test strategy (THRIFT-847).

          Apache Thrift Test Strategy

          Apache Thrift uses "make check" to drive language by language unit tests. These test each language's implementation of Apache Thrift for internal consistency and belong in thrift/lib/lang/test. Apache Thrift "make cross" drives cross language integration tests which insure languages can communicate with each other using a range of protocols and transports. These tests are found in thrift/test, along with other shared testing components (largely thrift IDL files). As this is a work in progress, many languages have no presence in the current make cross tests and still others have skewed make check test suites.

          Apache Thrift Browser JavaScript Test Structure

          JavaScript tests are currently organized into two suites.

          • ant thrift/lib/js/test/build.xml - These test JavaScript in the browser against a Java server. These tests require Java to be installed and also require the Java libs/tests to be built. Presently, to run these tests, you can run "./bootstrap.sh; ./configure; make check" at the thrift root, or if you have already done so in the past, you can run "make check" in thrift/lib/js/test to just run the JavaScript tests. Java must be installed when you run ./configure. Note: The cmake build does not yet support "make check" (1/5/2015).
          • grunt thrift/lib/js/Gruntfile.js - These test JavaScript in the Browser (using Phantom as the host) against JavaScript on the server (using NodeJS as the host). All of the JavaScript build tools (principally grunt and npm) are JavaScript based and do not require Java to be installed to work, though they do require Node.js. This test suite is more comprehensive than the build.xml suite, testing SSL, various jQuery arrangements, etc.

          Target JavaScript Test Structure

          Given that the Ant JavaScript to Java tests are cross language, these should be moved to the make check chain (integrated with thrift/test/test.sh/py, eliminating the thrift/lib/js/test/build.xml altogether). The grunt tests should then be wired into the make check tests as the go forward unit tests.

          Tests for a new protocol

          Any new protocol should have standard unit tests and a full set of integration tests with at least one of the two reference implementations (CPP/Java). In the case of JavaScript, this would imply a complete set of grunt driven tests against a Node.js server (adding TBinaryProtocol to the grunt test html files) and a "make cross" integration test with CPP or Java (wherein we should add tests for TBinaryProtocol and TJSONProtocol as well).

          Radoslaw Gruchalski If you can get the full suite of grunt tests (Browser to Node) to pass using TBinaryProtocol and the equivalent of the Ant tests to pass (Browser to Java) I will help you on the Apache Thrift build/test suite side.

          Best,
          Randy

          Show
          codesf Randy Abernethy added a comment - The JavaScript test suite is not perfectly aligned with the current Apache Thrift test strategy ( THRIFT-847 ). Apache Thrift Test Strategy Apache Thrift uses "make check" to drive language by language unit tests. These test each language's implementation of Apache Thrift for internal consistency and belong in thrift/lib/ lang /test. Apache Thrift "make cross" drives cross language integration tests which insure languages can communicate with each other using a range of protocols and transports. These tests are found in thrift/test, along with other shared testing components (largely thrift IDL files). As this is a work in progress, many languages have no presence in the current make cross tests and still others have skewed make check test suites. Apache Thrift Browser JavaScript Test Structure JavaScript tests are currently organized into two suites. ant thrift/lib/js/test/build.xml - These test JavaScript in the browser against a Java server. These tests require Java to be installed and also require the Java libs/tests to be built. Presently, to run these tests, you can run "./bootstrap.sh; ./configure; make check" at the thrift root, or if you have already done so in the past, you can run "make check" in thrift/lib/js/test to just run the JavaScript tests. Java must be installed when you run ./configure. Note: The cmake build does not yet support "make check" (1/5/2015). grunt thrift/lib/js/Gruntfile.js - These test JavaScript in the Browser (using Phantom as the host) against JavaScript on the server (using NodeJS as the host). All of the JavaScript build tools (principally grunt and npm) are JavaScript based and do not require Java to be installed to work, though they do require Node.js. This test suite is more comprehensive than the build.xml suite, testing SSL, various jQuery arrangements, etc. Target JavaScript Test Structure Given that the Ant JavaScript to Java tests are cross language, these should be moved to the make check chain (integrated with thrift/test/test. sh/py , eliminating the thrift/lib/js/test/build.xml altogether). The grunt tests should then be wired into the make check tests as the go forward unit tests. Tests for a new protocol Any new protocol should have standard unit tests and a full set of integration tests with at least one of the two reference implementations (CPP/Java). In the case of JavaScript, this would imply a complete set of grunt driven tests against a Node.js server (adding TBinaryProtocol to the grunt test html files) and a "make cross" integration test with CPP or Java (wherein we should add tests for TBinaryProtocol and TJSONProtocol as well). Radoslaw Gruchalski If you can get the full suite of grunt tests (Browser to Node) to pass using TBinaryProtocol and the equivalent of the Ant tests to pass (Browser to Java) I will help you on the Apache Thrift build/test suite side. Best, Randy
          Hide
          radekg Radoslaw Gruchalski added a comment - - edited

          I’d love to, it would be awesome to provide this to the community but the PhantomJS issue will prevent unicode characters decryption. The only way I can make this test to pass is to remove unicode characters from the test It work perfectly in the browser. I’m running this code here: https://github.com/radekg/gossiperl-client-chrome.

          Kind regards,

          Radek Gruchalski
          de.linkedin.com/in/radgruchalski/ (http://de.linkedin.com/in/radgruchalski/)

          Confidentiality:
          This communication is intended for the above-named person and may be confidential and/or legally privileged.
          If it has come to you in error you must take no action based on it, nor must you copy or show it to anyone; please delete/destroy and inform the sender immediately.

          Show
          radekg Radoslaw Gruchalski added a comment - - edited I’d love to, it would be awesome to provide this to the community but the PhantomJS issue will prevent unicode characters decryption. The only way I can make this test to pass is to remove unicode characters from the test It work perfectly in the browser. I’m running this code here: https://github.com/radekg/gossiperl-client-chrome . Kind regards,
 Radek Gruchalski de.linkedin.com/in/radgruchalski/ ( http://de.linkedin.com/in/radgruchalski/ ) Confidentiality: This communication is intended for the above-named person and may be confidential and/or legally privileged. If it has come to you in error you must take no action based on it, nor must you copy or show it to anyone; please delete/destroy and inform the sender immediately.
          Hide
          radekg Radoslaw Gruchalski added a comment -

          I've committed my simple binary test which visualises the PhantomJS problem: https://github.com/radekg/thrift/commit/e74c0d7c40fe3de9312ee571990fcbac8fa28249
          Not sure if this can be resolved but if so, I can find a bit of time to implement the integration test with the nodejs server.

          Show
          radekg Radoslaw Gruchalski added a comment - I've committed my simple binary test which visualises the PhantomJS problem: https://github.com/radekg/thrift/commit/e74c0d7c40fe3de9312ee571990fcbac8fa28249 Not sure if this can be resolved but if so, I can find a bit of time to implement the integration test with the nodejs server.
          Hide
          githubbot ASF GitHub Bot added a comment -
          Show
          githubbot ASF GitHub Bot added a comment - Github user radekg commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-68764557 And the code: https://github.com/radekg/thrift-js-binary-protocol
          Hide
          codesf Randy Abernethy added a comment -

          Hey Radek,

          I don't think we should let Phantom hold us back. It is not a practical environment, just a testing tool. We had to excise the WebSocket tests (testws.html) because Phantom did not support the current WebSocket spec. If you can get as many of the tests working as possible and move the others to a separate html, we can run the outliers manually until Phantom comes in line. We can also consider other options like Zombie or WebDriver.

          -Randy

          Show
          codesf Randy Abernethy added a comment - Hey Radek, I don't think we should let Phantom hold us back. It is not a practical environment, just a testing tool. We had to excise the WebSocket tests (testws.html) because Phantom did not support the current WebSocket spec. If you can get as many of the tests working as possible and move the others to a separate html, we can run the outliers manually until Phantom comes in line. We can also consider other options like Zombie or WebDriver. -Randy
          Hide
          radekg Radoslaw Gruchalski added a comment -

          Sounds good. This will have to wait until the weekend though. I'm sure I can do something about it!

          Show
          radekg Radoslaw Gruchalski added a comment - Sounds good. This will have to wait until the weekend though. I'm sure I can do something about it!
          Hide
          radekg Radoslaw Gruchalski added a comment -

          Randy, just to clarify - these tests are passing in the browser. Feel free to try from my branch. What would be the absolute minimum to get this in?

          Show
          radekg Radoslaw Gruchalski added a comment - Randy, just to clarify - these tests are passing in the browser. Feel free to try from my branch. What would be the absolute minimum to get this in?
          Hide
          codesf Randy Abernethy added a comment -

          Hey Radek,

          I will take a look at your patch as soon as I can later today.

          -Randy

          Show
          codesf Randy Abernethy added a comment - Hey Radek, I will take a look at your patch as soon as I can later today. -Randy
          Hide
          roger.meier Roger Meier added a comment -

          hey guys, please check this: https://github.com/ariya/phantomjs/issues/10476
          setting these environment variables fixed the issue:

          export LC_ALL=en_US.UTF-8
          export LANG=en_US.UTF-8
          export LANGUAGE=en_US.UTF-8
          

          the other thing I had in mind is defining a constant within test/ThriftTest.thrift containing the UTF-8 test data we use here https://github.com/apache/thrift/blob/master/lib/js/test/test.js#L55

          best!
          -roger

          Show
          roger.meier Roger Meier added a comment - hey guys, please check this: https://github.com/ariya/phantomjs/issues/10476 setting these environment variables fixed the issue: export LC_ALL=en_US.UTF-8 export LANG=en_US.UTF-8 export LANGUAGE=en_US.UTF-8 the other thing I had in mind is defining a constant within test/ThriftTest.thrift containing the UTF-8 test data we use here https://github.com/apache/thrift/blob/master/lib/js/test/test.js#L55 best! -roger
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user radekg reopened a pull request:

          https://github.com/apache/thrift/pull/345

          THRIFT-2926: JS: Binary protocol with unit tests.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/radekg/thrift js-binary-protocol

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/thrift/pull/345.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #345


          commit e79c300f09eb781bee5a2cc402bea0eb370ca2a4
          Author: radekg <radek@gruchalski.com>
          Date: 2015-01-02T16:12:45Z

          JS: Binary protocol with unit tests.

          commit e74c0d7c40fe3de9312ee571990fcbac8fa28249
          Author: radekg <radek@gruchalski.com>
          Date: 2015-01-05T19:28:23Z

          Integrated TBinaryProtocol into grunt tests to present the PhantonJS issue.


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user radekg reopened a pull request: https://github.com/apache/thrift/pull/345 THRIFT-2926 : JS: Binary protocol with unit tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/radekg/thrift js-binary-protocol Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/345.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #345 commit e79c300f09eb781bee5a2cc402bea0eb370ca2a4 Author: radekg <radek@gruchalski.com> Date: 2015-01-02T16:12:45Z JS: Binary protocol with unit tests. commit e74c0d7c40fe3de9312ee571990fcbac8fa28249 Author: radekg <radek@gruchalski.com> Date: 2015-01-05T19:28:23Z Integrated TBinaryProtocol into grunt tests to present the PhantonJS issue.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bufferoverflow commented on the pull request:

          https://github.com/apache/thrift/pull/345#issuecomment-69494103

          Thanks @radekg for this udate.

          any idea about the jslint issues?

          cd lib/js/test
          make check
          lib/js/test/build.xml:216: JSLint: 39 errors in 1 file

          They might be related to the unit test issues:
          ```
          cd lib/js/test
          ant unittest
          ...
          unittest:
          [echo] Running Unit Tests with headless browser!
          [exec] Xlib: extension "RANDR" missing on display ":99".
          [java] Timeout: killed the sub-process
          [java] Java Result: -1
          [exec] 'waitFor()' finished in 403ms.
          [exec] Tests completed in 267 milliseconds.
          [exec] 4 assertions of 41 passed, 37 failed.
          ```

          You can also remove ThriftTest.thrift file from this commit it is a duplication.

          Show
          githubbot ASF GitHub Bot added a comment - Github user bufferoverflow commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-69494103 Thanks @radekg for this udate. any idea about the jslint issues? cd lib/js/test make check lib/js/test/build.xml:216: JSLint: 39 errors in 1 file They might be related to the unit test issues: ``` cd lib/js/test ant unittest ... unittest: [echo] Running Unit Tests with headless browser! [exec] Xlib: extension "RANDR" missing on display ":99". [java] Timeout: killed the sub-process [java] Java Result: -1 [exec] 'waitFor()' finished in 403ms. [exec] Tests completed in 267 milliseconds. [exec] 4 assertions of 41 passed, 37 failed. ``` You can also remove ThriftTest.thrift file from this commit it is a duplication.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user radekg commented on the pull request:

          https://github.com/apache/thrift/pull/345#issuecomment-69494149

          On it and a broader discussion here: https://issues.apache.org/jira/browse/THRIFT-2926

          Show
          githubbot ASF GitHub Bot added a comment - Github user radekg commented on the pull request: https://github.com/apache/thrift/pull/345#issuecomment-69494149 On it and a broader discussion here: https://issues.apache.org/jira/browse/THRIFT-2926
          Hide
          codesf Randy Abernethy added a comment -

          Hey Guys,

          Where are we with this patch? I just tried to commit but the qunit:ThriftBinaryProtocol test still fails. I cleaned up the white space and did a subst to change the proto from BinaryProtocol to TBinaryProtocol.

          Also tried the UTF-8 exports Roger mentions.

          Radoslaw Gruchalski if you can mod the phantom tests to leave out the strings so that they pass and leave the full tests for manual browser use we can get this committed.

          Best,
          Randy

          Show
          codesf Randy Abernethy added a comment - Hey Guys, Where are we with this patch? I just tried to commit but the qunit:ThriftBinaryProtocol test still fails. I cleaned up the white space and did a subst to change the proto from BinaryProtocol to TBinaryProtocol. Also tried the UTF-8 exports Roger mentions. Radoslaw Gruchalski if you can mod the phantom tests to leave out the strings so that they pass and leave the full tests for manual browser use we can get this committed. Best, Randy
          Hide
          andrewdeandrade Andrew de Andrade added a comment -

          FWIW, this looks to only apply to the js implementation in lib/js. My intent is to get the nodejs code all working in the browser via browserify. I've already been able to implement this and test successfully (in this proof of concept commit**) in phantomjs. The only tests I cannot successfully run in phantomjs are the websockets tests because phantomjs 1.x.x doesn't implement the correct websocket specifications, but phantomjs 2 does and should be released shortly. The XHR proof of concept works flawlessly in phantomjs (I am using run-browser https://github.com/ForbesLindesay/run-browser ).

          If we get all the nodejs code working well in the browser, it's entirely possible that we could completely deprecate the implementation in lib/js unless someone has a strong reason to maintain that implementation. It's trivial to take the nodejs browser capable implementation and package it up so it produces a standalone file that exposes thrift as a global on the window object if that is something people want.

          Show
          andrewdeandrade Andrew de Andrade added a comment - FWIW, this looks to only apply to the js implementation in lib/js. My intent is to get the nodejs code all working in the browser via browserify. I've already been able to implement this and test successfully (in this proof of concept commit**) in phantomjs. The only tests I cannot successfully run in phantomjs are the websockets tests because phantomjs 1.x.x doesn't implement the correct websocket specifications, but phantomjs 2 does and should be released shortly. The XHR proof of concept works flawlessly in phantomjs (I am using run-browser https://github.com/ForbesLindesay/run-browser ). If we get all the nodejs code working well in the browser, it's entirely possible that we could completely deprecate the implementation in lib/js unless someone has a strong reason to maintain that implementation. It's trivial to take the nodejs browser capable implementation and package it up so it produces a standalone file that exposes thrift as a global on the window object if that is something people want. https://github.com/uber/thrift/commit/9ca44b40504cc8d45d0c1e47709128e474d991df (don't worry I'll break it up into smaller commits before submitting patches))
          Hide
          codesf Randy Abernethy added a comment -

          Hey Radoslaw Gruchalski,

          The tests for this patch are presently writting and reading from the protocol's internal buffer. This is fine, but usually something you can skip if you have end ot end tests working. You may note all of the other tests are end to end (client uses generated IDL code, serializes with proto, sends to transport, transmits to server, and back). This is really a requirement for any protocol, we need to have tests that show it working outright to know it works on initial commit and to avoid regression. After building a test web client and node server to actually use TBinaryProtocol in such a setting I discovered it never writes to the transport (for reference, the write is done in writeMessageEnd() in TJSONProtocol).

          I really wanted to get this in prior to jumping onto the several node patches waiting but it is not ready. If you can get the patch to lint clean with JSHint and run the test-async.js test suite clean I'll revisit (see: testws.html for an example).

          Best,
          Randy

          P.S. Andrew de Andrade, I will get the node/compact double patch in next and then start on your list.

          Show
          codesf Randy Abernethy added a comment - Hey Radoslaw Gruchalski , The tests for this patch are presently writting and reading from the protocol's internal buffer. This is fine, but usually something you can skip if you have end ot end tests working. You may note all of the other tests are end to end (client uses generated IDL code, serializes with proto, sends to transport, transmits to server, and back). This is really a requirement for any protocol, we need to have tests that show it working outright to know it works on initial commit and to avoid regression. After building a test web client and node server to actually use TBinaryProtocol in such a setting I discovered it never writes to the transport (for reference, the write is done in writeMessageEnd() in TJSONProtocol). I really wanted to get this in prior to jumping onto the several node patches waiting but it is not ready. If you can get the patch to lint clean with JSHint and run the test-async.js test suite clean I'll revisit (see: testws.html for an example). Best, Randy P.S. Andrew de Andrade , I will get the node/compact double patch in next and then start on your list.
          Hide
          andrewdeandrade Andrew de Andrade added a comment -

          Radoslaw,

          Given your work on TBinaryProtocol here, I figure you may be interested in the work I've done with getting browser support for both XHR and websocket connections in nodejs here:
          https://issues.apache.org/jira/browse/THRIFT-2976

          I have yet to get the TBinaryProtocol and TCompactProtocol working in the browser, but it's pretty close and I believe the remaining issues are related to how Int64's are handled.

          Show
          andrewdeandrade Andrew de Andrade added a comment - Radoslaw, Given your work on TBinaryProtocol here, I figure you may be interested in the work I've done with getting browser support for both XHR and websocket connections in nodejs here: https://issues.apache.org/jira/browse/THRIFT-2976 I have yet to get the TBinaryProtocol and TCompactProtocol working in the browser, but it's pretty close and I believe the remaining issues are related to how Int64's are handled.
          Hide
          beberg Adam Beberg added a comment -

          Any progress on this?

          Show
          beberg Adam Beberg added a comment - Any progress on this?
          Hide
          codesf Randy Abernethy added a comment -

          No updates I know of. If I'm missing a patch newer than the last one I commented on let me know.

          -Randy

          Show
          codesf Randy Abernethy added a comment - No updates I know of. If I'm missing a patch newer than the last one I commented on let me know. -Randy
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user ebremer commented on the issue:

          https://github.com/apache/thrift/pull/345

          Is there any further work being done to support TBinaryProtocol and TCompact in Javascript?

          Show
          githubbot ASF GitHub Bot added a comment - Github user ebremer commented on the issue: https://github.com/apache/thrift/pull/345 Is there any further work being done to support TBinaryProtocol and TCompact in Javascript?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user nsuke commented on the issue:

          https://github.com/apache/thrift/pull/345

          @ebremer none that I'm not aware of.
          You can use lib/nodejs code with browserify (or probably webpack), though.

          Show
          githubbot ASF GitHub Bot added a comment - Github user nsuke commented on the issue: https://github.com/apache/thrift/pull/345 @ebremer none that I'm not aware of. You can use lib/nodejs code with browserify (or probably webpack), though.
          Hide
          jking3 James E. King, III added a comment -

          This is the 3rd oldest pull request in the backlog. It would be nice to come to a decision on what to do with this. Does anyone want to pick it up and move forward? Do we want to deprecate lib/js in favor of lib/nodejs and decide not to add this?

          Show
          jking3 James E. King, III added a comment - This is the 3rd oldest pull request in the backlog. It would be nice to come to a decision on what to do with this. Does anyone want to pick it up and move forward? Do we want to deprecate lib/js in favor of lib/nodejs and decide not to add this?

            People

            • Assignee:
              codesf Randy Abernethy
              Reporter:
              radekg Radoslaw Gruchalski
            • Votes:
              1 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:

                Development