Allow overriding the post method
CharlieHess opened this issue · 24 comments
For clients running in an environment like Electron's main process, it becomes necessary to override the post method with something like the net module. It'd be nice to have a hook for doing this.
Sorry not sure I understand the question is there a problem w the vanilla node https impl to make these calls?
Yes, this is included in the link to the net module:
The following is a non-exhaustive list of why you may consider using the net module instead of the native Node.js modules:
Automatic management of system proxy configuration, support of the wpad protocol and proxy pac configuration files.
Automatic tunneling of HTTPS requests.
Support for authenticating proxies using basic, digest, NTLM, Kerberos or negotiate authentication schemes.
Support for traffic monitoring proxies: Fiddler-like proxies used for access control and monitoring.
Basically it lets you leverage Chromium's networking stack from a Node.js process.
sorry just trying to understand completely: are you are currently having a problem calling the slack api from electron? like, does it work (in our tests suite it seems to)
It works just fine, provided you are:
- Calling from the renderer process (aka a regular webpage in Electron) or
- You are not behind a proxy
But if you are using smallwins/slack from the main process your fate is tied to tiny-json-http, which does not handle proxies. So this is just about adding a hook to tell smallwins/slack hey, use this method for requests instead.
Ok, so what I'm hearing is I need to test and reproduce the failing the proxy case.
(I don't want to jump to the fix just yet because maybe we can bake that in without bringing alt http client libs concerns into the fold.)
Also! what would you want the interface to be if you did want to override the client http ? constructor param?
Sure, you could bake support for all possible proxy configurations into tiny-json-http, but then it's gonna become not-so-tiny-http. 😉 Chromium spent years getting all these working right. And the % of users who are in this situation is admittedly small.
Sooo for your own sanity it might be easier to just add a parameter like httpOverride or postOverride and allow consumers to drop in whatever request lib they want (and if they're using Electron, they can drop in the net module really easily!) Then you can use that as an escape hatch for folks in really advanced Enterprise situations, while still giving most folks the default tiny-json-http (and not ruining your life in the process – for testing NTLM proxies, for example, you will need to set up a Microsoft™ Active Directory™ Authentication Portal™).
Yeah constructor param would be great 🎉
quick thought---is it feasible to make this a runtime flag that decides to use the net module that electron packages?
eg.
const Slack = require('slack')
const slack = new Slack({electron:true})?
another fun idea 💡, what if tiny-json-http itself does a try/catch around {net} = require('electron') and falls back to node http/https?
would mean both a seamless upgrade and remaining still mostly tiny 😅
You could indeed, but I'd name it something like useElectronNet since just electron could be confusing (you could be an Electron app and happily using fetch from the renderer process).
Here's the check to see if you're running in Electron and in the main process:
return process && process.type === 'browser' && process.versions.electron;If that check passes you should be able to safely
const net = require('electron').net;(jinx! think we posted at the same time / check my prev reply / think we can get this in p quick once we decide best way)
Hmm, I'm still in favor of letting consumers override the request module. I think this might save folks in other extreme edge cases. Also I generally hate using try/catch for control flow 😝
WRT to try/catch I completely agree at a high level. However I think it could be a better choice for solving this problem with precision (Electron first class networking support) instead of solving a broader problem (maybe someone needs to use a different networking library).
Allowing an arbitrary userland HTTP client creates a rather epic surface for bugs that aren't in this module but would appear to be. (In some other libs I have experienced this regrettably often hence my hesitation.)
An aside: we both might want to get used to the idea of
try/catchflow control asasync/awaitsupport grows. Soon we'll be wrapping everything in 'em (which I def have mixed feelings about!)
That seems reasonable, although I'd still make it opt-in via flag on the off-chance someone explicitly wants Node.js' networking stack and doesn't care about proxies.
That's a good point. An then no try/catch funk since we can assume the user knows that setting will blow up in non Electron places.
I'll cut a branch w a ctor option {useElectronNet:true} that opts into that code path and pingback here. ✨
There are some things to watch out for if you want to swap in the net.request object at this level. Its API is different in a couple ways:
https://github.com/electron/electron/blob/master/docs/api/client-request.md#new-clientrequestoptions
🆒 thx that helps (will def need your help baking this as I have zero electron network experience thus far!)
just realizing that if this is happening at runtime we'll have to tweak the browser build to ignore that module; which is nbd, but maybe at that point we should consider making this a build option instead of a runtime option. this does mean consuming this library for this use case isn't a simple npm install anymore which I don't know if thats a big deal to electron devs?
since tiny-json-http is lightweight I think it's OK to keep this out of the build configuration. IMO it's still preferable to get this with an npm install – that's still the way Electron devs do most everything.
update: have this sorta mocked out in a branch here https://github.com/smallwins/slack/tree/electron but my test runner is failing because electron.net is undefined! think it could be an issue w https://github.com/juliangruber/browser-run
my guess is browser-run uses Electron's renderer process (so, basically just Chrome).
Best way I know of to mock Electron's main process is https://github.com/jprichardson/electron-mocha#run-tests. But you could fake it as a Node environment with some extra globals.
Holy crap that was an adventure! I got it working tho!
Not hugely confident in that code path yet and would appreciate some help testing. You can see the PR here:
Thanks for responding so quickly! 🎉
Thx for reporting and Electron guidance @CharlieHess! 🙏 Closing this. Will merge the PR early next week after a bit more testing, docs and general polish. (I think we lost a few points on code coverage which I'd like to regain. 🤓)