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

axios 0.17 support & NodeJS current LTS support (8) #5

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

soswow
Copy link

@soswow soswow commented Nov 8, 2017

Changes required to have it working with axios 0.17, + while I was there I've bumped code to proper modern level, introduced linting, simplified some tests, etc

If client wants to do CORS GET request via xhr adapter - it fails,
because custom headers require additional OPTIONS handshake and everything
brakes.
@nettofarah
Copy link
Owner

Hi, @soswow.
Thanks for putting in the time and helping improve axios-vcr.
A few points:

  • I tend to write my OSS using ES5 like syntax in order to broaden the support I can offer in terms of older versions of Node. I think it is actually ok to use a more modern syntax though since Node LTS has changed recently. But I would prefer that we used a different PR for that.

  • As for 6a818ac
    Are we still covering the original problem I ran into when I added those comments and that implementation?

  • As for 6a818ac
    what's your motivation and use-case here?

I would prefer to see a separate PR for this, so I can understand the use-case, provide feedback and document the new feature.
Also, by merging this feature first, we could issue a non-breaking release of axios-vcr that would allow current users of the library to get this feature without having to do a full upgrade.

  • As for adding ESLint and converting the code, I would also prefer to have a separate discussion about that. I tend to prefer using prettier and standard as my formatting and linter, so all my codebases are consistent with the same style.

Please don't take my feedback as criticism of your work. I'm very happy to see more people interested in my library 😄

Copy link
Owner

@nettofarah nettofarah left a comment

Choose a reason for hiding this comment

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

See previous comments

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

Successfully merging this pull request may close these issues.

2 participants