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

Allow setting of maxBuffer for packing external modules #96

Closed
wants to merge 4 commits into from

Conversation

mhodgson
Copy link
Contributor

@mhodgson mhodgson commented Feb 1, 2017

I received the following error when trying to compile in Docker: stderr maxBuffer exceeded. It stems from the npm-programatic module and its use of exec when we install external dependencies. I have a PR opened there to add a setting for maxBuffer: Manak/npm-programmatic#8

I don't recommend merging this until that PR is accepted, but I wanted to make this PR in the meantime for anyone else that runs into this problem.

This PR adds a setting called packExternalModulesMaxBuffer that is passed to the npm exec for installing external dependencies.

@mhodgson
Copy link
Contributor Author

mhodgson commented Feb 5, 2017

OK, the PR on npm-programmatic was accepted so this now uses a non-forked version. Ready to merge.

@yashafromrussia
Copy link

Running into a similar issue. I'm currently using @mhodgson's fork that works perfectly. Hey @thenikso, any plans on merging this soon?

@jelder
Copy link

jelder commented Jun 2, 2017

@thenikso The other PR (Manak/npm-programmatic#8) has been merged. Think we could get this one merged soon, too?

@HyperBrain HyperBrain mentioned this pull request Jun 30, 2017
package.json Outdated
@@ -31,7 +31,8 @@
"body-parser": "^1.15.2",
"express": "^4.14.0",
"fs-extra": "^0.26.7",
"npm-programmatic": "0.0.5"
"npm-programmatic": "^0.0.7",
"webpack": "^1.13.1"
Copy link
Contributor

@hassankhan hassankhan Jun 30, 2017

Choose a reason for hiding this comment

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

We shouldn't hardcode a Webpack dependency since #83 means a user has to install Webpack as a peer dependency.

@HyperBrain
Copy link
Member

HyperBrain commented Jul 31, 2017

@mhodgson Can you update (rebase this PR) onto the current master? I think the issue still is present on Docker as nothing changed and the npm-programmatic version in master is still 0.0.5?

@gorjuspixels Can you confirm that the issue still exists with serverless-webpack 2.2.0?

@mhodgson
Copy link
Contributor Author

@HyperBrain all set.

@HyperBrain
Copy link
Member

@mhodgson Great 👍 ! I will check it out tomorrow and do some tests (with master and the 3.0.0 branch).
If everything works well, I'll merge it into master.

@HyperBrain
Copy link
Member

Tests against master, v3.0.0 and v3.0.0-individual-packaging were successful. I also noticed a speed-up while packaging.

The PR is ready to be merged -> nevertheless I will wait. Depending on the status (quality/testing) of the individual packaging branch the merge target might be the v3.0.0 branch. If everything goes well, that will be released next week.

@HyperBrain
Copy link
Member

@mhodgson If you check the "enable edits by maintainers" checkbox in the PR, I can add some README changes to the PR and retarget it before I'll merge it next week.

@mhodgson
Copy link
Contributor Author

mhodgson commented Aug 2, 2017

@HyperBrain checked.

@HyperBrain HyperBrain changed the base branch from master to v3.0.0 August 4, 2017 21:01
@HyperBrain
Copy link
Member

@mhodgson All v3 features are now merged into the v3.0.0 branch. Unfortunately the merge left this PR with conflicts. IMO the easiest way would be to create a new PR with these changes (it's just 2 lines of code) from the latest upstream v3.0.0 branch. Can you do that, so that I can get it into v3?

@mhodgson
Copy link
Contributor Author

mhodgson commented Aug 8, 2017

@HyperBrain done: #185

@mhodgson mhodgson closed this Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants