[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/1175 sorry @jeking3, I don't have the bandwidth these days to pick this back up ---
[GitHub] thrift pull request #1175: THRIFT-4064 Update node library dependencies
Github user zertosh closed the pull request at: https://github.com/apache/thrift/pull/1175 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/1175 I'm at a loss here. I can't see this PR through. Sorry everyone, and thank you for your time. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1175: THRIFT-4064 Update node library dependencies
Github user zertosh commented on a diff in the pull request: https://github.com/apache/thrift/pull/1175#discussion_r101392209 --- Diff: build/docker/debian/Dockerfile --- @@ -113,8 +113,8 @@ RUN apt-get update && apt-get install -y --no-install-recommends \ neko-dev \ libneko0 -# Node.js dependencies - THRIFT-4064 says it must be >= 4.x -RUN curl -sL https://deb.nodesource.com/setup_4.x | bash - +# Node.js dependencies - THRIFT-4064 says it must be >= 0.12.0 --- End diff -- The point was to move away from `ws@<1.0.0`, since those versions depend on native node modules - they're a huge pain (https://github.com/apache/thrift/pull/672#issuecomment-276678791). Ideally we'd upgrade to the latest `ws` (v2.x), but the newer JS syntax is proving to be really problematic. Less ideally, but nonetheless solves the native module problem, is to upgrade to `ws@^1.0.0`. That only requires Node >= 0.12.0, and doesn't use newer syntax. I still want to use `ws@^2.0.0`, but that requires other upstream dep fixes. I'm not really familiar with phantomjs, so that's going to take me a bit. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/1175 @bgould that's a very similar use case to ours. Also the Q library is kinda heavy, and could probably switch to just using native Promises. For the purposes of this PR (avoid binary deps), I just switched to ws@1.x instead of ws@2.x, so that phantomjs doesn't choke. We'll still have some lib duplication, but at least you don't need to compile the dep. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/1175 @jeking3 pretty sure that the "SyntaxError: Parse error" error is phanthomjs (via run-browser) choking on the new syntax. The tests are running phantomjs@1.9.20 (https://travis-ci.org/apache/thrift/jobs/200386738#L2252), but it seems that it's v2 that added ES6 support (see https://github.com/ariya/phantomjs/issues/14506). Alternatively, how do you feel about making the `require` call for this module configurable?(https://github.com/apache/thrift/blob/e1832c354391deb0e0ce94a62ff32e8ce1c83fd3/compiler/cpp/src/thrift/generate/t_js_generator.cc#L412) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/1175 The tests are failing because `run-browser`'s browserify can't find `utf-8-validate` (and it won't find `bufferutil` either. Those are optional dependencies of `ws`. You can't seem to configure `run-browser` to tell it to ignore those modules. So I'm going to try adding them to devDeps. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #1175: THRIFT-4064 Update node library dependencies
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/1175 @jeking3 I don't really know what to change exactly in the Dockerfile :/ --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request #1175: Update node library dependencies
GitHub user zertosh opened a pull request: https://github.com/apache/thrift/pull/1175 Update node library dependencies * The changes to node-int64 and Q are small: - https://github.com/broofa/node-int64/compare/v0.3.3...v0.4.0 - https://github.com/kriskowal/q/compare/v1.0.1...v1.4.1 * ws under went a rewrite between 0.4.32 and 2.0.1 (https://github.com/websockets/ws/compare/0.4.32...2.0.1). The API as used by thrift is backwards compatible. However, ws now uses ES6 syntax, so it requires at least Node 4. - Most notable change here is that ws no longer tries to install binary modules. * Node 4 is now the minimally required version of Node, due to ws' requirement. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zertosh/thrift THRIFT-4064 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1175.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 #1175 commit face8c95163fc1b382ce7522b64334547fd43eca Author: Andres Suarez Date: 2017-02-01T19:37:45Z Update node library dependencies * The changes to node-int64 and Q are small: - https://github.com/broofa/node-int64/compare/v0.3.3...v0.4.0 - https://github.com/kriskowal/q/compare/v1.0.1...v1.4.1 * ws under went a rewrite between 0.4.32 and 2.0.1 (https://github.com/websockets/ws/compare/0.4.32...2.0.1). The API as used by thrift is backwards compatible. However, ws now uses ES6 syntax, so it requires at least Node 4. - Most notable change here is that ws no longer tries to install binary modules. * Node 4 is now the minimally required version of Node, due to ws' requirement. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/672 @jfarrell will do. Made an issue https://issues.apache.org/jira/browse/THRIFT-4064 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Github user zertosh commented on the issue: https://github.com/apache/thrift/pull/672 Thanks @jfarrell! @modocache, unfortunately we still can't use this lib. The version of `ws` that it uses is crazy old (v0.4.32 from Aug 2014). Any version prior to [ws@1.0.0](https://github.com/websockets/ws/releases/tag/1.0.0) requires that you compile a few of its third-party deps - major blocker for using internally at FB (toolchain problems, node ABI issues, etc). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] thrift pull request: Fix package.json to include only the needed f...
Github user zertosh commented on the pull request: https://github.com/apache/thrift/pull/672#issuecomment-158733257 Between 0.9.2 and 0.9.3, the npm release [removed node-unit](https://github.com/apache/thrift/commit/d8187c5ff1d8b83d170cbce69282688be39df19c#diff-b9cfc7f2cdf78a7f4b91a753d10865a2) from it's deps (a 27MB install), but gained all these extra files (a different 27MB). Is a 0.9.4 release near with this fix? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
