You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jkrems <gi...@git.apache.org> on 2015/10/30 21:35:39 UTC
[GitHub] thrift pull request: Fix package.json to include only the needed f...
GitHub user jkrems opened a pull request:
https://github.com/apache/thrift/pull/672
Fix package.json to include only the needed files
Before:
```
> du -sh node_modules/thrift
28M node_modules/thrift
```
After:
```
> mkdir -p /tmp/thrift-module
> cp -r node_modules/thrift/lib/nodejs/{lib,README.md} /tmp/thrift-module
> du -sh /tmp/thrift-module
208K /tmp/thrift-module
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jkrems/thrift jk-fix-npm-package
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/672.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 #672
----
commit e744a32fcea8c4867a4269dc58d5fdd22dffe406
Author: Jan Krems <ja...@groupon.com>
Date: 2015-10-30T20:32:29Z
Fix package.json to include only the needed files
Before:
```
> du -sh node_modules/thrift
28M node_modules/thrift
```
After:
```
> mkdir -p /tmp/thrift-module
> cp -r node_modules/thrift/lib/nodejs/{lib,README.md} /tmp/thrift-module
> du -sh /tmp/thrift-module
208K /tmp/thrift-module
```
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Posted by zertosh <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Fix package.json to include only the needed f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/672
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Fix package.json to include only the needed f...
Posted by zertosh <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Fix package.json to include only the needed f...
Posted by jkrems <gi...@git.apache.org>.
Github user jkrems commented on the pull request:
https://github.com/apache/thrift/pull/672#issuecomment-153830391
If it makes a difference - I already [signed the CLA before in a different context](https://github.com/dscape/nano/issues/265#issuecomment-82564981). Not sure if the process around this changed. In any case: Happy to accept the contribution license.
For reference, these are the instructions for Github PRs:
* Create a fork for http://github.com/apache/thrift
* Create a branch for your changes(best practice is issue as branch name, e.g. THRIFT-9999)
* Modify the source to include the improvement/bugfix
* Remember to provide tests for all submited changes
* When bugfixing: add test that will isolate bug before applying change that fixes it
* Verify that you follow Thrift Coding Standards (you can run 'make style', which ensures proper format for some languages)
* Verify that your change works on other platforms by adding a GitHub service hook to Travis CI and AppVeyor
* Commit and push changes to your branch (please use issue name and description as commit title, e.g. THRIFT-9999 make it perfect)
* Issue a pull request with the jira ticket number you are working on in it's name
* Wait for other contributors or committers to review your new addition
* Wait for a committer to commit your patch
It doesn't look like those are mentioning any licensing topics. Admittedly I ignored the JIRA parts because it felt like overkill for a small change.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Posted by modocache <gi...@git.apache.org>.
Github user modocache commented on the issue:
https://github.com/apache/thrift/pull/672
+1 for @zertosh's comment. @bufferoverflow, is there any way for me, a third-party contributor, to help you or other maintainers publish a new version on npm? I'd love to use a version without these unneeded dependencies. Plus, the npm version is 0.9.3, but the Apache Thrift website only provides direct links to 0.10.0 downloads.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Posted by jfarrell <gi...@git.apache.org>.
Github user jfarrell commented on the issue:
https://github.com/apache/thrift/pull/672
done
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Posted by jfarrell <gi...@git.apache.org>.
Github user jfarrell commented on the issue:
https://github.com/apache/thrift/pull/672
@zertosh if you can submit a patch to update the version of ws in use we can look at turning around a 0.11.0 to help out
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Fix package.json to include only the needed f...
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the pull request:
https://github.com/apache/thrift/pull/672#issuecomment-153511243
Thanks for the PR!
We need a strong YES to according to the contribution guidelines as mentioned here https://thrift.apache.org/docs/HowToContribute (generated from https://github.com/apache/thrift/blob/master/CONTRIBUTING.md)
I guess CONTRIBUTING.md is displayed when creating a merge request? But there is no ack... or is pressing the create Merge Request an ACK on the contribution license?
We probably need to simplify this somehow especially for such small but essential contributions.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Posted by bufferoverflow <gi...@git.apache.org>.
Github user bufferoverflow commented on the issue:
https://github.com/apache/thrift/pull/672
@jfarrell you are the npm publisher, could you please push 0.10.0 to the npm registry?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #672: Fix package.json to include only the needed files
Posted by zertosh <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---