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

Fixing defaults to use httpAdapter if available #1285

Merged
merged 2 commits into from Apr 11, 2018

Conversation

@mividtim
Copy link
Contributor

commented Jan 11, 2018

Default to the httpAdapter if process.env.HOME is a string (if we're in a Node environment). This makes the httpAdapter the default in Electron or in a Node environment with an XMLHttpRequest polyfill.

Addresses #1284

@DeroSavage

This comment has been minimized.

Copy link

commented Jan 11, 2018

For me this is not working. It gives me "adapter is not a function"

@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

What is your environment, @DeroSavage ?

@DeroSavage

This comment has been minimized.

Copy link

commented Jan 11, 2018

@mividtim I'm using node 9.3.0 and executing my script with "node index.js"

@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

Installing Node 9.3.0 now...

@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

Running typeof process !== 'undefined' && process.env && typeof process.env.HOME === 'string' in Node 9.3.0 returns true. Which index.js are you testing with? node index.js from the root of this branch returns no output. I'm not sure what the expected result is, or if this is what you are testing.

@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

@DeroSavage I cannot reproduce your issue. Could you please provide some sample code that fails? I'd love to get this merged soon. Thanks!

@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

Does this project have any active maintainers? I have two open PRs, passing tests, improving functionality, and not a peep for almost a month, after several pings.

@emilyemorehouse emilyemorehouse force-pushed the axios:master branch 2 times, most recently from 2760755 to 48a7902 Feb 19, 2018

@heisian

This comment has been minimized.

Copy link

commented Feb 28, 2018

@emilyemorehouse

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

@mividtim I've got this on my radar now.

The current implementation isn't sufficient since process.env.HOME is not consistently used across operating systems and isn't guaranteed to be defined.

I'd suggest checking out detect-node's method or doing some research on Node-specific environment variables to handle compatibility (for example, I know that process.env.NODE_VERSION is now used, though I'm not sure if it has always been set for all older versions of Node).

@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

Thanks, @emilyemorehouse ! I just pushed up the change to use the same method as the file you referenced. It checks out in my environment, so I'm totally happy with this. Your diligence is appreciated. :-)

@emilyemorehouse emilyemorehouse merged commit 0b3db5d into axios:master Apr 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

Thanks, Emily! Any tips on how I can know when my two axios PRs are published in an axios release?
https://github.com/axios/axios/pulls?q=is%3Apr+author%3Amividtim+is%3Aclosed

@mividtim mividtim deleted the mividtim:mividtim/use-http-in-electron branch Apr 11, 2018

@mividtim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

@emilyemorehouse Oh, I forgot. My other PR was already released in 0.18.0. Please tag me when this is released! I'd much appreciate it!

@emilyemorehouse

This comment has been minimized.

Copy link
Member

commented Apr 11, 2018

Will do!
I think the best way currently is to check the changelog, especially if you have a specific PR # you're checking on.

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