Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Remove unnecessary node-gyp header install logic#845

Merged
winstliu merged 13 commits intomasterfrom
wl-direct-header-install
Jun 11, 2019
Merged

Remove unnecessary node-gyp header install logic#845
winstliu merged 13 commits intomasterfrom
wl-direct-header-install

Conversation

@winstliu
Copy link
Copy Markdown
Contributor

@winstliu winstliu commented May 12, 2019

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

Currently, any time we have a command that does something related to npm install, we ensure that the Electron header versions are correctly installed by spawning a process to run node-gyp install with the correct Electron version, header URL, etc. It turns out that node-gyp will actually do this for us automatically as long as you supply the correct arguments to the relevant npm commands. Yay!

Alternate Designs

None.

Benefits

There's pretty much no node-gyp code in the codebase now -- it's all handled through npm.

Possible Drawbacks

I don't really see any.

Verification Process

All the specs pass, which depend on node-gyp finding and downloading the correct header files.

Applicable Issues

None, but can be seen as a followup to #832

Todo

  • Yank out proxy logic into a helper function for further de-duplication

Copy link
Copy Markdown
Contributor

@rafeca rafeca left a comment

Choose a reason for hiding this comment

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

Nice @50Wliu ! This looks good to me and the less code we have to handle things that other tools can handle for us the better 😄

Merge and publish whenever suits better for you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants