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

Do not swallow errors in case npm ls fails without an empty stdout and use spawn for child processes #373

Merged
merged 4 commits into from Apr 25, 2018

Conversation

Projects
None yet
2 participants
@HyperBrain
Copy link
Member

HyperBrain commented Apr 25, 2018

What did you implement:

If npm ls fails the plugin tries to analyze stderr for npm errors that can be ignored. If none are found, it will pass stdout to JSON.parse().
The case that any non-npm error was happening was not handled correctly (e.g. if the stdout maxbuffer size got exceeded by a very big dependency section). The plugin passed the empty stdout to JSON parse and failed there without telling the exact reason for the failure.
Additionally, Node's buffered stdio streams do not work reliably and the exec method is not meant to be used for processes that might return data of arbitrary length.

How did you implement it:

The packager executables are now executed with child_process.spawn which allows for stdio streams of arbitrary length. On execution failure, a proper SpawnError is now thrown that contains the full stdout and stderr streams.
Additionally this eliminates the need of the packExternalModulesMaxBuffer buffer configuration which has been removed from the README.
The implementation should seamlessly work as before.

How can we verify it:

Use a package.json with many dependencies, so that the output of npm ls exceeds the max stdout buffer default of 200k with the old serverless-webpack version.
It should work with this branch now.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@HyperBrain HyperBrain changed the title Do not swallow errors in case npm ls fails without an empty stdout Do not swallow errors in case npm ls fails without an empty stdout and use spawn for child processes Apr 25, 2018

@HyperBrain HyperBrain added this to the 5.1.2 milestone Apr 25, 2018

Utils unit tests
Enhance error message

Removed deprecated option from README

Convert all outstanding changes (yarn and package)

Adjusted NPM unit tests

Use spawn in any npm commands

Use spawn instead of exec

Fix unit test description

Do not swallow errors in case npm ls fails without a proper stdout

@HyperBrain HyperBrain force-pushed the error-on-stdout-empty-with-npm branch from c9548bc to b379de9 Apr 25, 2018

HyperBrain added some commits Apr 25, 2018

@HyperBrain HyperBrain merged commit 81f6696 into master Apr 25, 2018

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 92.879%
Details

@HyperBrain HyperBrain deleted the error-on-stdout-empty-with-npm branch Apr 25, 2018

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Apr 25, 2018

Hi @franciscocpg . Could you do me a favor and test if the current master still works properly for you? I switched the packager invocations to use child_process.spawn instead of exec here, but only verified Windows yet. It would be great if I had an additional success report before I release the fix 😃 . Thanks.

@franciscocpg

This comment has been minimized.

Copy link
Member

franciscocpg commented Apr 25, 2018

hi @HyperBrain
I'll do it.

A sls package would be enough?

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Apr 25, 2018

Thanks 😏 . Yes I think so, because it runs through the whole npm or yarn processes. There's no need to actually deploy it.

@franciscocpg

This comment has been minimized.

Copy link
Member

franciscocpg commented Apr 25, 2018

@HyperBrain
It works seamlessly in one of our biggest project with npm process.

I'm unable to run using yarn. See #370 (comment).

@HyperBrain

This comment has been minimized.

Copy link
Member Author

HyperBrain commented Apr 25, 2018

Thanks for the tests 👍 . I commented on the issue with yarn. Maybe the plugin could emit a better error message for that case.

@franciscocpg

This comment has been minimized.

Copy link
Member

franciscocpg commented Apr 25, 2018

Was able to test with yarn using the workaround I've mentioned in #370 (comment). It worked seamlessly too.

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