replace axiosInstance with getFetchClient inside the admin #15245
Conversation
simotae14
added
source: core:admin
Codecov ReportBase: 59.11% // Head: 59.07% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #15245 +/- ##
==========================================
- Coverage 59.11% 59.07% -0.04%
==========================================
Files 1502 1501 -1
Lines 38360 38365 +5
Branches 7385 7384 -1
==========================================
- Hits 22677 22666 -11
- Misses 13411 13427 +16
Partials 2272 2272
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -38,7 +39,7 @@ const fetchStrapiLatestRelease = async (toggleNotification) => { | |||
|
|
|||
| const fetchAppInfo = async () => { | |||
| try { | |||
| const { data, headers } = await axiosInstance.get('/admin/information'); | |||
| const { data, headers } = await get('/admin/information'); | |||
There was a problem hiding this comment.
Could you please make sure all calls prefixed with /admin take process.env.STRAPI_ADMIN_BACKEND_URL into account?
There was a problem hiding this comment.
We can't use just the env var, we should be using the internal API method to fetch the admin config, eg in both these examples your env var wouldn't work
There was a problem hiding this comment.
Thanks guys, this activity will be part of another ticket just to focus on the axiosInstance replacement on this one. This issue #13889 will be fixed in another ticket with some activities in the BE and in the FE if needed
simotae14
requested review from
Bassel17,
christiancp100,
gu-stav and
derrickmehaffy
|
Added to my list to review as I want to check to make sure reverse proxying isn't impacted negatively. |
|
Thanks @derrickmehaffy, please let me know the results of your review. Thanks |
|
@simotae14 I don't know if you have discussed that already internally but I'd prefer if we start to release your branches as soon as they are ready instead of batching them into another branch. The reason would be that we should start to get this on the road brick by brick instead of a big bang release to: 1) limit the impact and therefore bugs 2) catch early on in case this becomes a problem for plugins developers (which I don't think). We could start with the one from users & permissions right after today's 4.6. release and get it into 4.6.1 - this allows us to have it in What do you think? |
|
Yes, @gu-stav I agree with your idea, at the beginning I thought was useful to have a main branch (enhancement/axios-migration-step-2) with all the code of the others branches just to remind me to refactor the code, for example, to remove the useCallback and useMemo...but honestly is better to merge piece by piece every branch in main just to double check, as you said, the behavior before the releases |
simotae14
changed the base branch from
enhancement/axios-migration-step-2
to
main
| [fetchData, params, slug, toggleNotification] | ||
| ); | ||
|
|
||
| const handleConfirmDeleteData = useCallback( | ||
| async (idToDelete) => { | ||
| try { | ||
| await axiosInstance.delete(getRequestUrl(`collection-types/${slug}/${idToDelete}`)); | ||
| await fetchClient.del(getRequestUrl(`collection-types/${slug}/${idToDelete}`)); |
There was a problem hiding this comment.
Same here
const {del} = useFetchClient()
| [formatMessage, getData, getDataSucceeded, notifyStatus, push, toggleNotification] | ||
| ); | ||
|
|
||
| const handleConfirmDeleteAllData = useCallback( | ||
| async (ids) => { | ||
| try { | ||
| await axiosInstance.post(getRequestUrl(`collection-types/${slug}/actions/bulkDelete`), { | ||
| await fetchClient.post(getRequestUrl(`collection-types/${slug}/actions/bulkDelete`), { |
There was a problem hiding this comment.
Why don't you use the object destructuring in the del method like in the other ones? It's nothing important, feel free to ignore it 😂
const { post } = useFetchClient()
There was a problem hiding this comment.
I needed the fetchClient variable to execute the get method because we also imported the lodash get method
| @@ -26,7 +27,7 @@ const useRolesList = (shouldFetchData = true) => { | |||
|
|
|||
| const { | |||
| data: { data }, | |||
| } = await axiosInstance.get('/admin/roles'); | |||
| } = await fetchClient.get('/admin/roles'); | |||
There was a problem hiding this comment.
I needed the fetchClient variable to execute the get method because we also imported the lodash get method
There was a problem hiding this comment.
The eslint disabling isn't great for me to be honest, it's a quick win and it'll lead to bugs if someone works on the codebase (which we will) before the problem is addressed. It shouldn't be too hard to make the hook have a stable function identity.
I stopped highlighting them all after i realised how many there were.
| @@ -186,6 +187,7 @@ const CollectionTypeFormWrapper = ({ allLayoutData, children, slug, id, origin } | |||
| return () => { | |||
| source.cancel('Operation canceled by the user.'); | |||
| }; | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
Please don't disable this it causes more problems down the line when people edit the function but don't change their deps, if you can't safely put the fetchclient as a dep you need to change the code in the hook to reflect this.
| @@ -237,6 +237,8 @@ const CollectionTypeFormWrapper = ({ allLayoutData, children, slug, id, origin } | |||
| return Promise.reject(err); | |||
| } | |||
| }, | |||
| // TODO: remove when we evaluate the need of the useCallback | |||
| // eslint-disable-next-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
this again, im not a fan of slapping eslint rules as an easy win it's really bad practice.
There was a problem hiding this comment.
ok I understand but as I said in the comment, I will remove it in this activity https://strapi-inc.atlassian.net/browse/DX-570
if you are not ok with it I need to use again the getFetchClient
There was a problem hiding this comment.
I don't understand why you don't think you can include it in the deps? It's a stable function because it lives outside of react no?
There was a problem hiding this comment.
Looking at the hook quickly if anything you need to wrap the object you return in a useMemo? But we can't have this many eslint disables around hook deps, we'll be shooting ourselves in the foot.
There was a problem hiding this comment.
I am fine with your comment, I can try to solve my problem in a different way. Just to keep in mind, there are other places, into the code, where we are using the eslint comment
There was a problem hiding this comment.
Just to keep in mind, there are other places, into the code, where we are using the eslint comment
Then we should as a collective try get rid of them 🤝
There was a problem hiding this comment.
could be an interesting discussion for the Frontend sync @simotae14 @joshuaellis
we should work on enforcing some general rules from now and forward
simotae14
dismissed stale reviews from christiancp100 and Bassel17
via
a513f3d
simotae14
requested review from
joshuaellis,
gu-stav,
Bassel17 and
christiancp100
|
I am missing the point here why did you got back to |
|
@simotae14 Could you try and delete the back-and-forth commits please before merging this PR? Otherwise you could also squash it, but I think it doesn't make sense to have all of it in the git history. |
|
Thanks Gustav, yes there are too many commits, then I prefer to do a Squash and merge on this case |
simotae14
dismissed stale reviews from joshuaellis, gu-stav, christiancp100, and derrickmehaffy
via
c395813
| const { result } = await setup(); | ||
|
|
||
| expect(result.current).toHaveProperty('get'); | ||
| expect(result.current).toHaveProperty('post'); | ||
| expect(result.current).toHaveProperty('put'); | ||
| expect(result.current).toHaveProperty('del'); | ||
|
|
||
| expect(result.current.get).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
Mh, I think if you want to make that test useful you'd have to re-render and confirm it still has only be called once, because right now this is expected no?
There was a problem hiding this comment.
yes sorry, I push the first draft of the test, now I have added the rerendering and if the get is called once even after the rerendering
There was a problem hiding this comment.
I don't think this test is reliable or correct. You're testing against the renderHook instance of useFetchClient, you're not testing against the instance of TestComponent. Because wrapper just wraps adds a component around the internal test component of @testing-library/react-hooks...
You should just be using @testing-library/react's render method to render the component with the hook inside and assert against that by spying/mocking the hook.
…seFetchClient and getFetchClient in the admin
simotae14
force-pushed
the
enhancement/axios-migration-admin
branch
from
2f9e08a
to
ea15154
Compare
What does it do?
Replace every instance of axiosInstance with the new useFetchClient and getFetchClient in the Admin
We have already implemented the new constructs useFetchClient and getFetchClient in this PR
Now we need to replace every place where we use axiosInstance with the new constructs, test the possible regression errors and still provide the old implementation of the axiosInstance into the helper plugin with a message of the removal in v5
Why is it needed?
Inside the Strapi project we have a lot of different implementations of the same code to use axios and its axiosInstance. In this branch we replace axiosInstance in the admin
How to test it?
You need to use the new construct inside every axiosInstance call and check the possible regression errors
List of the API calls in the Admin where we use the new construct and how to test them: