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

Update the 'Global axios defaults' README to use a safer example #3471

Conversation

aliclark
Copy link
Contributor

@aliclark aliclark commented Dec 10, 2020

The existing example usage it isn't safe in the general case as it can
lead to auth tokens being leaked to 3rd party endpoints by unexpectedly.

This change instead gives an example using
"axios.defaults.headers.common" to set the User-Agent, which is an
equally helpful use-case to document.

The 'Custom instance defaults' example just below the 'Global axios
defaults' example shows a method to set the 'Authorization' header
specific to a given API. I've also updated the variable in the 'Custom
instance defaults' code to use a semantically more relevant name within
that example.

The existing example usage it isn't safe in the general case as it can
lead to auth tokens being leaked to 3rd party endpoints by unexpectedly.

This change instead gives an example using
"axios.defaults.headers.common" to set the User-Agent, which is an
equally helpful use-case to document.

The 'Custom instance defaults' example just below the 'Global axios
defaults' example shows a method to set the 'Authorization' header
specific to a given API. I've also updated the variable in the 'Custom
instance defaults' code to use a semantically more relevant name within
that example.
@aliclark aliclark force-pushed the default-headers-example-without-confidential-text branch from aa839a4 to 3933e16 Compare December 12, 2020 18:23
@chinesedfan
Copy link
Collaborator

Thanks for your sweet updates. Note that usually, auth tokens will be calculated in every request by the send value with a special algorithm, instead of a fixed value. And instance is a good name for the section Custom instance defaults.

@aliclark
Copy link
Contributor Author

Hi @chinesedfan

Thanks for your sweet updates

As a first time contributor, I find this wording unprofessional and derogatory, in the context of a PR being closed.

I find this in contradiction to what's found in the axios' Code of Conduct so I'm calling this out to @rubennorte @mzabriskie @emilyemorehouse @nickuraltsev who are the listed people for the axios organization.

Note that usually, auth tokens will be calculated in every request by the send value with a special algorithm

This is incorrect. A search shows Google's access token expiration time to be 1 hour, and Facebook's access token expiration time to be 60 days.

And instance is a good name for the section Custom instance defaults.

In my opinion this is not welcoming and inclusive language in response to a good faith suggestion. It ignores my rationale for providing the change.

@chinesedfan
Copy link
Collaborator

Sorry for my inaccurate English as a nonnative speaker. I want to clarify that I didn't mean to be derogatory at all.

But I will keep the decision to reject this PR. Other maintainers can feel free to give more acceptable explanations or different opinions.

@aliclark
Copy link
Contributor Author

Sorry for my inaccurate English as a nonnative speaker

I would suggest axios find another nonnative speaker of the same native language to weigh in on this.

But I will keep the decision to reject this PR. Other maintainers can feel free to give more acceptable explanations or different opinions.

For the record, I would like another maintainer to take a fresh look at this PR.

@jasonsaayman
Copy link
Member

Hi @aliclark,

Thanks for the contribution and sorry for the above problems. Sorry about the language used but @chinesedfan is not a native English speaker and this causes some odd idiosyncrasies with the way some of his written English comes across, he does however have the best intentions and has worked very hard on the Axios repo over a long period of time.

Since the other maintainers that you have mentioned above will likely not reply to this message as Ruben, Matt and Nick have not been active on Axios for quite some time and Emily is pretty busy with other projects, I will weigh in on the changes.

Firstly the change to the naming of the variable I think is probably not a good idea as we are dealing with an instance of Axios here and these are global variables being added to that instance. So the naming for clarity should stay instance. As for the other change, I think we can add this example rather than removing the current example as there are use cases where one would like to set the auth token on the instance.

If you can adjust and push I will accept the PR.

Thanks

@aliclark
Copy link
Contributor Author

Hi @jasonsaayman

Thanks for weighing in on this PR.

Sorry about the language used but @chinesedfan is not a native English speaker and this causes some odd idiosyncrasies with the way some of his written English comes across, he does however have the best intentions and has worked very hard on the Axios repo over a long period of time.

I appreciate he is a very helpful contributor, and that my offence taken may not have been his original intentions. Nonetheless I have a hard time seeing what he intended to communicate, if not condescension, as someone who doesn't speak the same native language.

I could see how he might have meant the equivalent of "Thanks for your kind updates" or "Thanks for your lovely updates". Those still seem a little condescending or weirdly abrupt to me in the context of the communication though. Like I said, I'll hold judgement on it until a native speaker of the same language can comment.

Firstly the change to the naming of the variable I think is probably not a good idea as we are dealing with an instance of Axios here and these are global variables being added to that instance. So the naming for clarity should stay instance.

Okay, I respect the decision to keep the name generic for a global variable and more closely match the axios terminology.

As for the other change, I think we can add this example rather than removing the current example as there are use cases where one would like to set the auth token on the instance.

I've kept the Authorization example and added a small comment about the security risk, hope that's ok. I'm happy to reword it, but I think this is a potential foot-gun worth mentioning to users of the library.

@jasonsaayman
Copy link
Member

jasonsaayman commented Jan 11, 2021

Hi @aliclark,

Thanks, I cannot speak to this:

I appreciate he is a very helpful contributor, and that my offence taken may not have been his original intentions. Nonetheless I have a hard time seeing what he intended to communicate, if not condescension, as someone who doesn't speak the same native language.

Other than some languages and cultures having a more direct approach than others, coming from my country South Africa there is a language Afrikaans and if you translated the comment that would be a pretty acceptable way for you to communicate your point, maybe even looked at as a nice way of saying something, that being said I cannot deduce the intentions of what is written as text is a weak medium for communication that often leads to issues like the above.

Thanks for the re-word, I had one last comment that I left on the PR for you, I am a bit over the top when it comes to clarity and how things are phrased. In all this is a good addition as I do understand what you are trying to communicate here and since Axios seems to be found by a lot of people making their first in-roads into a dev career this seems like a very valid addition to docs as a lot of these developers do not understand those risks.

Thanks for your contribution and I encourage you to open more pull requests on the repo and to be on the lookout for the Axios documentation site that will be coming very soon, maybe we can look at how to highlight some of the caveats and pitfalls of Axios on this new site :)

@aliclark
Copy link
Contributor Author

Thanks Jason.

Apologies if that is the case, I think I was triggered both by not understanding the intended sentiment and the quickness of the PR closing. Hopefully I can contribute helpfully again to axios in future.

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.

3 participants