Skip to content
This repository has been archived by the owner on Mar 31, 2021. It is now read-only.

Migrate from request to axios http client #29

Closed
wants to merge 3 commits into from

Conversation

apaul314
Copy link

@apaul314 apaul314 commented Apr 6, 2017

Related to snowplow/snowplow#3182.
Axios is http client with ability to run under different
environments such as browser or node. Axios is promise based but emitter
still takes single callback function due to the backward compatibility.

Related to snowplow/snowplow#3182.
Axios is http client with ability to run under different
environments such as browser or node. Axios is promise based but emitter
still takes single callback function due to the backward compatibility.
@coveralls
Copy link

coveralls commented Apr 6, 2017

Coverage Status

Coverage decreased (-8.6%) to 91.429% when pulling 77096ef on hose314:master into fb36091 on snowplow:master.

@snowplowcla
Copy link

Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://github.com/snowplow/snowplow/wiki/CLA to learn more and sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

@apaul314
Copy link
Author

apaul314 commented Apr 6, 2017

@snowplowcla I signed it!

@snowplowcla
Copy link

Confirmed! @hose314 has signed the Individual Contributor License Agreement. Thanks so much

@snowplowcla snowplowcla added cla:yes and removed cla:no labels Apr 6, 2017
@coveralls
Copy link

coveralls commented Apr 10, 2017

Coverage Status

Coverage decreased (-8.5%) to 91.549% when pulling 6d8eca7 on hose314:master into fb36091 on snowplow:master.

@apaul314
Copy link
Author

Have no idea why but all tests passed locally

@chuwy
Copy link
Contributor

chuwy commented Apr 10, 2017

From my experience with JavaScript tracker this happens because of some version incompatibilities.

  • everything worked fine with Node 0.10 (from travis.yml) with old dependencies when we first published it
  • everything works fine with latest Node (which you use) and latest dependencies (which are installed by npm)
  • but everything fails with Node 0.10 (from travis.yml) and latest dependencies

@coveralls
Copy link

coveralls commented Apr 10, 2017

Coverage Status

Coverage decreased (-8.5%) to 91.549% when pulling 92bfd19 on hose314:master into fb36091 on snowplow:master.

@apaul314
Copy link
Author

Okay, i will try to figure out it on node 0.10. Thanks

@chuwy
Copy link
Contributor

chuwy commented Apr 10, 2017

No problems! If you're going to bump Node, dependencies, tweak travis.yml or do other not axios-related stuff, please do it as a separate commit. Thank you very much.

@apaul314
Copy link
Author

axios/axios#837 (comment)

Two options here:

  1. remove node 0.10 support from snowplow
  2. close this pull and support own emmiter

@chuwy
Copy link
Contributor

chuwy commented Apr 12, 2017

I'm okay with removing node 0.10, it's ancient. However, I'm also okay with "own emitter" (snowplow/snowplow-scala-tracker#46), but not for this PR/release.

@apaul314
Copy link
Author

@chuwy okay, then i close this PR and implement own emitter on steroid)) If someone also will encounter the same issue, please mention me and i will make new PR.

@apaul314 apaul314 closed this Apr 12, 2017
@chuwy
Copy link
Contributor

chuwy commented Apr 12, 2017

@hose314 by "own emitter not for this PR" I meant that I like idea of having separate emitters in general. If last thing we need to do to make this PR working is to remove NodeJS 0.10 then it's fine. Sorry for confusion.

@AlanFoster
Copy link

It would be great to have support for an axios emitter, as it would unblock react-native support for this library 👍

@Producdevity
Copy link

This PR is a year old and for my understanding it was already reviewed. Any chance this is going to be merged anytime soon? Like @AlanFoster mentioned, this would make react native support possible! :)

@alexanderdean
Copy link
Member

alexanderdean commented Oct 18, 2018

I think @hose314 closed the PR. Could you make a new PR, Andrey?

@alexanderdean alexanderdean removed the request for review from chuwy October 18, 2018 09:32
@apaul314
Copy link
Author

Hi everyone,
I'm no longer writing js I can reopen this PR but not quite sure that the code is not outdated.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants