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

Add redirect support for downloading node.lib #620

Merged
merged 2 commits into from
Nov 13, 2020

Conversation

adumbidiot
Copy link
Contributor

I added primitive redirect support for downloading node.lib in NAPI since Electron uses it. While its nice not pulling in any external Rust or Js deps in the build script, I think some time should be spent at some point making the download process more robust.

@kjvalencik
Copy link
Member

Thanks for the contribution and helping us to test electron. Using node is a neat idea, but it might be complicated enough that we should bring in a build dependency. For example, this doesn't handle 301s.

@goto-bus-stop what do think of using ureq?

@adumbidiot
Copy link
Contributor Author

Looking at the history, ureq used to be used until it got replaced.

@kjvalencik
Copy link
Member

kjvalencik commented Oct 29, 2020

@adumbidiot That commit was part of the same PR that also replaced it. It's never been used in neon. My preference is to use ureq instead of maintaining an increasingly complicated JS script in a string.

Also, interestingly, node isn't actually required to build a neon n-api module. I'm not sure it's realistic that a user would want to build in an environment without it, but except for this download, it's possible.

@goto-bus-stop
Copy link
Member

I think some time should be spent at some point making the download process more robust.

I agree … initially we got rid of the dependency since I assumed we'd just be getting things from nodejs.org, but since that is not the case, the ureq dependency will be worth it. We might be able to do it just by reverting that commit you linked, if that still applies cleanly…

@goto-bus-stop
Copy link
Member

Reverted 2574b2d on top of this branch, that should do the trick 🙏

@goto-bus-stop goto-bus-stop merged commit 8142074 into neon-bindings:main Nov 13, 2020
@goto-bus-stop
Copy link
Member

Thanks for bringing this up @adumbidiot :)

@kjvalencik kjvalencik mentioned this pull request Nov 16, 2020
@adumbidiot adumbidiot deleted the node-lib-redirect branch March 7, 2021 23:37
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.

3 participants