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

temp fix for axios #279

Merged
merged 2 commits into from
Sep 20, 2023
Merged

temp fix for axios #279

merged 2 commits into from
Sep 20, 2023

Conversation

huang0h
Copy link
Contributor

@huang0h huang0h commented Sep 19, 2023

Checklist

  • 1. Run yarn run check
  • 2. Run yarn run test

Why

Resolves #

This PR

Screenshots

Verification Steps

@chromium-52 chromium-52 self-requested a review September 20, 2023 01:06
chromium-52
chromium-52 previously approved these changes Sep 20, 2023
Copy link
Member

@chromium-52 chromium-52 left a comment

Choose a reason for hiding this comment

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

👍 I wonder if the only reason why the bug is happening is just because the code that adds the request and response interceptors aren't being run at all, could something like this work?

/* in `src/App.tsx` */
const App: React.FC = () => {
  useEffect(() => addAxiosInterceptors(), []);
}

/* in `src/auth/axios.ts` */
// lines 40 through 97 of `src/App.tsx` in this PR

export const addAxiosInterceptors = () => {
  AppAxiosInstance.interceptors.response.use( ... );

  AppAxiosInstance.interceptors.request.use((config) => { ... });
}

Still p ugly but at least we're all learning something 🤷 (don't make auth more complicated than it needs to be)

@huang0h
Copy link
Contributor Author

huang0h commented Sep 20, 2023

👍 I wonder if the only reason why the bug is happening is just because the code that adds the request and response interceptors aren't being run at all, could something like this work?

/* in `src/App.tsx` */
const App: React.FC = () => {
  useEffect(() => addAxiosInterceptors(), []);
}

/* in `src/auth/axios.ts` */
// lines 40 through 97 of `src/App.tsx` in this PR

export const addAxiosInterceptors = () => {
  AppAxiosInstance.interceptors.response.use( ... );

  AppAxiosInstance.interceptors.request.use((config) => { ... });
}

Still p ugly but at least we're all learning something 🤷 (don't make auth more complicated than it needs to be)

This only worked 100% on my end if I called addAxiosInterceptors() outside of the component function - when I had it in useEffect, the reports page still gave the same error. For some reason, only the calls to get reports didn't have X-Access-Token even though other admin calls like emailer searching, force unadopting, and adding sites had the header and worked totally fine, and I have no idea why.

@chromium-52
Copy link
Member

This only worked 100% on my end if I called addAxiosInterceptors() outside of the component function - when I had it in useEffect, the reports page still gave the same error. For some reason, only the calls to get reports didn't have X-Access-Token even though other admin calls like emailer searching, force unadopting, and adding sites had the header and worked totally fine, and I have no idea why.

Ah I think this could be because useEffects get run after a render. This means that when we have addAxiosInterceptors() in a useEffect() in App.tsx and we render the App.tsx component, we'll first render everything that gets returned by it first and only after that run the useEffect(). This isn't an issue for calls like emailer searching and force unadopting because those calls get made after the page gets rendered. But when we go to the reports page, we can see here that we make requests to the backend in the page's useEffect(). Sadly, as we can see in the console logs in the screenshot below (link to sandbox just as an fyi), useEffects in child components get run before those in the parent components and so we are adding the interceptors after we make the requests to get the adoption and stewardship info and getting an error

image

With this being said idk what the best way to get around this would be so I'd say let's just merge this and spend an hour or so to see if we can move the addAxiosInterceptors() call to somewhere outside of App.tsx. If we can't, then we can just leave this as is and learn to not implement auth like this in the future

@huang0h
Copy link
Contributor Author

huang0h commented Sep 20, 2023

This only worked 100% on my end if I called addAxiosInterceptors() outside of the component function - when I had it in useEffect, the reports page still gave the same error. For some reason, only the calls to get reports didn't have X-Access-Token even though other admin calls like emailer searching, force unadopting, and adding sites had the header and worked totally fine, and I have no idea why.

Ah I think this could be because useEffects get run after a render. This means that when we have addAxiosInterceptors() in a useEffect() in App.tsx and we render the App.tsx component, we'll first render everything that gets returned by it first and only after that run the useEffect(). This isn't an issue for calls like emailer searching and force unadopting because those calls get made after the page gets rendered. But when we go to the reports page, we can see here that we make requests to the backend in the page's useEffect(). Sadly, as we can see in the console logs in the screenshot below (link to sandbox just as an fyi), useEffects in child components get run before those in the parent components and so we are adding the interceptors after we make the requests to get the adoption and stewardship info and getting an error

image

With this being said idk what the best way to get around this would be so I'd say let's just merge this and spend an hour or so to see if we can move the addAxiosInterceptors() call to somewhere outside of App.tsx. If we can't, then we can just leave this as is and learn to not implement auth like this in the future

Ah that makes sense, thanks for the explanation!

@huang0h huang0h merged commit 8252df1 into master Sep 20, 2023
@huang0h huang0h deleted the AH.TempAxiosFix branch September 20, 2023 04:33
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