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

Moved build dependencies to devDependencies #73

Closed
wants to merge 2 commits into from

Conversation

YasserAA
Copy link

This fix is to reduce the size of the node modules by moving build dependencies to devDependencies.
Related issue #52

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@michaelwclark
Copy link
Contributor

@YasserAA have you signed a CLA? If not, I can reproduce this and do the PR from my end. We are dependant on this fix before we are in full production mode on our serverless project.

@TheLudd
Copy link

TheLudd commented Aug 23, 2018

Should not also everything related to eslint, gulp and jshint also be moved to devDependencies?

@michaelwclark If you can speed this up with your own PR, please do. This causes problems for us as well.

@bertero
Copy link

bertero commented Aug 23, 2018

@TheLudd you are correct. All those files should go to devDep.

Is this PR ever going to go through? It's been a while already...

@michaelwclark
Copy link
Contributor

@bertero None have since May. The dev cycle of this project is too slow for me. I'm forking and republishing under my company scope. I'll update everyone when this is finished in the next day or so. Hopefully this will get the attention of FB devs and perhaps allow me to get on as maintainer of this project. At that point I'll be updating weekly here, until then I will be doing so on our fork.

@YasserAA
Copy link
Author

Sorry for the late reply, I have signed the CLA now.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@michaelwclark
Copy link
Contributor

I have decided to publish this under our company scope. I will maintain PRs weekly to our repo and will PR back in to this master at the same frequency until maintainers here are able to keep up with community demand. https://github.com/denimlabs/facebook-nodejs-business-sdk
npm i @denimlabs/facebook-nodejs-business-sdk

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codytwinton has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@codytwinton codytwinton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@codytwinton
Copy link
Contributor

Thank you all for your patience. We're a small team working on the SDKs, but we're doing our best to move these SDKs forward.

I'll merge this into our internal repo and it should be available for the next release.

@TheLudd
Copy link

TheLudd commented Aug 28, 2018

@codytwinton @YasserAA please see my comment regarding this PR before merging:

Should not also everything related to eslint, gulp and jshint also be moved to devDependencies?

All these dependencies should be moved, they also cause problems when they are located under regular dependencies.

@YasserAA
Copy link
Author

@TheLudd They are in devDependencies, check the code.
"dependencies": { "mixwith": "~0.1.1", "request": "^2.81.0", "request-promise": "~4.1.1", },

@TheLudd
Copy link

TheLudd commented Aug 28, 2018

Ok, looks good for me as well then.

@TheLudd
Copy link

TheLudd commented Sep 11, 2018

Any progress here? If all is well can the merge be made and a related release?

@michaelwclark
Copy link
Contributor

@TheLudd Should be g2g, Iv'e been using this PR in my fork for several weeks without issue. As far as I know we were just waiting on you guys to do release.

@TheLudd
Copy link

TheLudd commented Sep 12, 2018

@michaelwclark Do you have a public released version of that fork that I can use as well?
PS I am not a part of Facebook and not involved in merge/release of this.

@michaelwclark
Copy link
Contributor

@TheLudd yes sir. sorry I just noticed this. npm i @denimlabs/facebook-nodejs-business-sdk
https://github.com/denimlabs/facebook-nodejs-business-sdk

@michaelwclark
Copy link
Contributor

@YasserAA this should be closed. The team has since implimented this in current build. @codytwinton @bertero or @YasserAA please close this.

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

Successfully merging this pull request may close these issues.

None yet

6 participants