Skip to content
This repository was archived by the owner on Feb 13, 2024. It is now read-only.

Feature: Allow passing axios instance or config in options. #240

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

krisdages
Copy link
Contributor

Background:

I have an Electron app using axios for HTTP where I am using a custom adapter to get around proxy issues by using Electron's net module.

My workaround:

The current version of analytics-node doesn't expose any configuration for axios, so I added the following options properties:

  • axiosConfig: configuration options for creating an axios instance if you don't have axios as a dependency
  • axiosInstance: instance of axios if you already have axios as a dependency, maybe with a different version

It's not ideal from a library-coupling perspective, but I didn't see an easy way to avoid that without
defining some library-agnostic interface for the HTTP adapter which would still support retries, which seems overkill.

I just thought I would post this PR in case it was a useful enhancement.

Alternative considered:

One option axios with either a config object or a function returning an axios instance.

* @property {Number} [flushAt] (default: 20)
* @property {Number} [flushInterval] (default: 10000)
* @property {String} [host] (default: 'https://api.segment.io')
* @property {Boolean} [enable] (default: true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those updates

Copy link
Contributor

@pbassut pbassut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we merge this we can work out #246 by using axios' baseURL parameter(and also leaving feature like proxy intact).

We can merge this and I'll tackle that later. Leaving this here for reference

@pooyaj
Copy link
Contributor

pooyaj commented Feb 11, 2021

@krisdages @pbassut could you please fix the conflict so we can merge this?

@pbassut
Copy link
Contributor

pbassut commented Feb 11, 2021

Sure. Notice we don't need to merge #246 if we merge this.
I would even prefer to merge this as this provides more flexibility.

@pbassut
Copy link
Contributor

pbassut commented Feb 11, 2021

Also, I'll have to create another PR to fix the conflicts here.

@pooyaj
Copy link
Contributor

pooyaj commented Feb 11, 2021

@pbassut oh, sorry just merged the other one. Agree, this would've covered that.

@pbassut
Copy link
Contributor

pbassut commented Feb 11, 2021

#270

@pooyaj pooyaj merged commit f483c64 into segmentio:master Feb 11, 2021
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.

3 participants