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

Add pass auth config to endpoints #30

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

Conversation

vhsiebe
Copy link
Contributor

@vhsiebe vhsiebe commented May 28, 2024

Momenteel sturen we de authentication header met elke call mee als we een auth token hebben, dit zorgt ervoor dat sommige calls niet kunnen gecached worden.
vanaf nu zal je moeten meegeven of het auth nodig heeft of niet.

Aangezien die verschillende zaken kan breken wordt dit een major version, en moeten we onze grootste applicaties aanpassen (stemtool, aanmeldmoment)

@vhsiebe vhsiebe requested review from a team, DefV, pjaspers, mitchellklijs and casperboone and removed request for a team May 28, 2024 07:42
@vhsiebe vhsiebe requested a review from DefV May 30, 2024 06:39
if (!this.radioToken) {
throw new Error('No currentUserToken available')
}
api.requestModifiers.push((request) => request.withHeader('Authorization', `Bearer ${this.radioToken}`))

Choose a reason for hiding this comment

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

De requestModifiers voelt als een private ding (maar ik zie ook niet meteen waarom we deze bijhouden in een array), dus ik had hier verwacht dat je het ging zetten op een this.passAuth of iets, zodat je dat in de request() method weer op zou kunnen bouwen.

(Wat ik zeg doet eigenlijk exact hetzelfde als wat jij hier doet, dus dit is geen 'mijn manier is de goede' meer een 'wat denk je van dit?'-comment)

Copy link

@pjaspers pjaspers left a comment

Choose a reason for hiding this comment

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

👍

@@ -24,8 +24,9 @@ export default class Endpoint {
* @param {requestCallback} callback
* @returns {object}
*/
async requestData(callback, key = 'data') {
const response = await callback(this.api.request())
async requestData(callback, key = 'data', config = { withAuth: false }) {
Copy link
Member

Choose a reason for hiding this comment

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

Misschien is het een idee om in plaats van een nieuw config object een (optionele) Api instance mee te geven. Dan hebben we minder punten waarop er verandering is.

(dus bijv. this.requestPaginatedData((r) => r.get('/members/me/ratings'), 'ratings', this.api.withAuth()))

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.

5 participants