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

replace axiosInstance with getFetchClient inside the users-permissions #15350

Merged
merged 31 commits into from
Jan 26, 2023

Conversation

simotae14
Copy link
Contributor

@simotae14 simotae14 commented Jan 5, 2023

What does it do?

Replace every instance of axiosInstance with the new useFetchClient and getFetchClient in the users-permissions plugin

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 users-permissions plugin

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:
Here you can see a list of API calls changed with the instructions to test them:

…lace the api call with the getFetchClient get method
@simotae14 simotae14 added source: plugin:users-permissions Source is plugin/users-permissions package pr: enhancement This PR adds or updates some part of the codebase or features labels Jan 5, 2023
@simotae14 simotae14 self-assigned this Jan 5, 2023
@simotae14 simotae14 changed the title replace axiosInstance in the Roles ListPage fetchData function to rep… replace axiosInstance with getFetchClient inside the users-permissions Jan 5, 2023
@simotae14 simotae14 marked this pull request as draft January 5, 2023 10:59
@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Base: 58.60% // Head: 58.60% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (e3bc1cd) compared to base (15247d1).
Patch coverage: 70.58% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15350   +/-   ##
=======================================
  Coverage   58.60%   58.60%           
=======================================
  Files        1508     1507    -1     
  Lines       38702    38696    -6     
  Branches     7463     7462    -1     
=======================================
- Hits        22682    22679    -3     
+ Misses      13698    13696    -2     
+ Partials     2322     2321    -1     
Flag Coverage Δ
back 47.33% <ø> (ø)
front 66.12% <70.58%> (+<0.01%) ⬆️
unit_back 47.33% <ø> (ø)
unit_front 66.12% <70.58%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...plugins/users-permissions/admin/src/utils/index.js 100.00% <ø> (ø)
...missions/admin/src/pages/Roles/CreatePage/index.js 74.07% <50.00%> (ø)
...ermissions/admin/src/pages/Roles/EditPage/index.js 80.35% <50.00%> (ø)
...ions/admin/src/pages/AdvancedSettings/utils/api.js 77.77% <66.66%> (-5.56%) ⬇️
...ssions/admin/src/pages/EmailTemplates/utils/api.js 77.77% <66.66%> (-5.56%) ⬇️
...permissions/admin/src/pages/Providers/utils/api.js 54.54% <66.66%> (+4.54%) ⬆️
...ssions/admin/src/pages/Roles/ListPage/utils/api.js 50.00% <66.66%> (+4.54%) ⬆️
...-permissions/admin/src/hooks/useFetchRole/index.js 74.07% <100.00%> (ø)
...rs-permissions/admin/src/hooks/usePlugins/index.js 82.14% <100.00%> (ø)

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

…place the api call with the getFetchClient del method
…ace the api call with the getFetchClient get method
…ls to replace the api call with the getFetchClient put method
…ailTemplate to replace the api calls with the getFetchClient get and put method
…e api call with the getFetchClient put method
@simotae14 simotae14 marked this pull request as ready for review January 9, 2023 10:50
Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

I haven't tested all the files but the changes look good to me.

There's just one thing that's not clear to me. If I understand correctly:

  • we use useFetchClient in React components or custom hooks
  • we use getFetchClient in "non-react" functions, so everywhere else

But couldn't we use getFetchClient everywhere, and not need useFetchClient? Since it's a plain function it should work in both React and non-react functions. Does useFetchClient have a performance benefit over getFetchClient?

@joshuaellis
Copy link
Member

I haven't tested all the files but the changes look good to me.
There's just one thing that's not clear to me. If I understand correctly:
we use useFetchClient in React components or custom hooks
we use getFetchClient in "non-react" functions, so everywhere else
But couldn't we use getFetchClient everywhere, and not need useFetchClient? Since it's a plain function it should work in both React and non-react functions. Does useFetchClient have a performance benefit over getFetchClient?

@remidej fair question. The hook manages lifecycle stuff so for example if the component is unmounted then the AbortController will cancel the request :) this is the main difference and imo an important helper.

@gu-stav
Copy link
Contributor

gu-stav commented Jan 12, 2023

Yeah good question: I'd argue we should avoid getFetchClient whenever it is possible and not at all when a request is triggered by a react component. Long term this would allow us to work on more high-level hooks that compose e.g. useFetchClient and add e.g. error handling ... to it.

Copy link
Contributor

@gu-stav gu-stav left a comment

Choose a reason for hiding this comment

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

Almost done ✨ One axiosInstance down.

gu-stav
gu-stav previously approved these changes Jan 20, 2023
Copy link
Contributor

@gu-stav gu-stav left a comment

Choose a reason for hiding this comment

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

Looks good to me! I haven't tested it functionally though. Good work! 🚀

joshuaellis
joshuaellis previously approved these changes Jan 20, 2023
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

⭐ solid

@simotae14 simotae14 changed the base branch from enhancement/axios-migration-step-2 to main January 25, 2023 09:17
@simotae14 simotae14 dismissed stale reviews from joshuaellis and gu-stav January 25, 2023 09:17

The base branch was changed.

@simotae14 simotae14 added this to the 4.6.1 milestone Jan 26, 2023
@simotae14 simotae14 merged commit ccf5e84 into main Jan 26, 2023
@simotae14 simotae14 deleted the enhancement/axios-migration-users-permissions branch January 26, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: enhancement This PR adds or updates some part of the codebase or features source: plugin:users-permissions Source is plugin/users-permissions package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants