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

fix: improve error handling in npm install #21589

Merged
merged 2 commits into from Dec 30, 2019

Conversation

@dsanders11
Copy link
Contributor

dsanders11 commented Dec 20, 2019

Description of Change

This improves a couple of current shortcomings in the error handling, namely:

  • Any error in downloadArtifact (such as a network error or 404) would result in an unhandled promise rejection
  • Node still exits with exit code 0 in the case of unhandled promise rejection, so it would look like it succeeded in those cases
  • An error during extraction or writing path.txt would still be considered 'good install' if you run the script again, leaving things in a state which doesn't self-heal

This PR should fix those issues.

cc @MarshallOfSound

Checklist

Release Notes

Notes: no-notes

@dsanders11 dsanders11 requested a review from electron/wg-releases as a code owner Dec 20, 2019
Copy link
Member

codebytere left a comment

thanks for this! just a few comments before we should be set to move forward 🚀

npm/install.js Show resolved Hide resolved
npm/install.js Outdated Show resolved Hide resolved
@electron-cation electron-cation bot removed the new-pr 🌱 label Dec 21, 2019
@jkleinsc

This comment has been minimized.

Copy link
Contributor

jkleinsc commented Dec 30, 2019

Merging as CI failure is unrelated to PR change.

@jkleinsc jkleinsc merged commit 85382d8 into electron:master Dec 30, 2019
8 of 9 checks passed
8 of 9 checks passed
appveyor: win-x64-testing-pr AppVeyor build failed
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Dec 30, 2019

No Release Notes

codebytere added a commit that referenced this pull request Dec 31, 2019
* fix: improve error handling in npm install

* chore: simplify logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.