Skip to content

chore: fix axios dependency #405

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

Closed
wants to merge 1 commit into from
Closed

Conversation

lance
Copy link
Member

@lance lance commented Apr 20, 2021

Somehow axios had been completely removed from the dependencies in package.json. I guess our tests still worked because @types/axios must have a dependency on axios, and by the transitive power of npm dependencies it was there. But we actually have axios in the package itself. So, users who want to use the axios emitter that we ship will have to know to also install axios without this change.

Signed-off-by: Lance Ball [email protected]

@lance lance added the chore/dependencies Pull requests that update a dependency file label Apr 20, 2021
@lance lance requested a review from a team April 20, 2021 15:17
@lance lance self-assigned this Apr 20, 2021
@lholmquist
Copy link
Contributor

I didn't realize we still had an axios emitter. I thought we were trying to move away from including any specific protocol libraries.

@lance
Copy link
Member Author

lance commented Apr 20, 2021

I didn't realize we still had an axios emitter.

import { Message, Options } from "../..";
import axios from "axios";
export function axiosEmitter(sink: string) {
return function (message: Message, options?: Options): Promise<unknown> {
options = { ...options };
const headers = {
...message.headers,
...(options.headers as Record<string, string>),
};
delete options.headers;
return axios.post(sink, message.body, {
headers: headers,
...options,
});
};
}

I thought we were trying to move away from including any specific protocol libraries.

Yes we are. But we forgot to actually remove it for 4.0.

The BEFORE functionality still exists until the release of 4.x at which time it will be removed.

#342

I mean, I guess we could leave it as-is and consider it "removed" - though it is still accessible, and could work fine as long as the user declares the axios dependency themselves.

@lance
Copy link
Member Author

lance commented Apr 20, 2021

Closing this for now. We can revisit later if we need to.

@lance lance closed this Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore/dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants