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

Remove explicit dependency on node-gyp#832

Merged
rafeca merged 22 commits intomasterfrom
wl-rm-node-gyp-dep
Apr 26, 2019
Merged

Remove explicit dependency on node-gyp#832
rafeca merged 22 commits intomasterfrom
wl-rm-node-gyp-dep

Conversation

@winstliu
Copy link
Copy Markdown
Contributor

@winstliu winstliu commented Apr 2, 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

This dependency was added in #730 as a way to always know how to access the node-gyp binary. However, npm has been bumped several times since then without touching node-gyp, leaving it quite outdated. I believe it should be fine to remove this dependency and simply rely on the version npm ships with.

Alternate Designs

Update it to the version npm ships with.

Benefits

Less packages to maintain. This should also unblock using Visual Studio 2017 build tools.

Possible Drawbacks

npm's directory flattening scheme may change, which will require us to update the node-gyp search path. I think if it does change, we'll notice it fairly quickly and be able to change the path without too much hassle.

Verification Process

None yet.

Applicable Issues

None.

@lee-dohm
Copy link
Copy Markdown
Contributor

lee-dohm commented Apr 9, 2019

Can you publish a pre-release version of apm and open a PR on Atom to use that new version in order to confirm that we get a green build? Contact me in Slack if you have any questions.

@vinkla
Copy link
Copy Markdown

vinkla commented Apr 20, 2019

@50Wliu could this pull request solve atom/atom#18540?

@winstliu
Copy link
Copy Markdown
Contributor Author

Perhaps? I don't really know; apm's internals are quite complicated and I'm still figuring out how things work and why they were designed this way.

@winstliu
Copy link
Copy Markdown
Contributor Author

Some of these changes weren't the cleanest, specifically all the node-gyp path updating. I'm wondering if we should just try updating our version of node-gyp rather than depending on npm's.

Winston Liu added 2 commits April 20, 2019 18:05
@rafeca
Copy link
Copy Markdown
Contributor

rafeca commented Apr 26, 2019

Thanks for doing this! Since the PR on atom/atom using the prerelease version passes, I'm confident about merging it.

@50Wliu can you take care of publishing a stable release afterwards and updating the atom/atom PR to point to the stable release?

@rafeca rafeca merged commit f207ebd into master Apr 26, 2019
@rafeca rafeca deleted the wl-rm-node-gyp-dep branch April 26, 2019 18:44
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