Skip to content

feat!: replace xhr provider with @nextcloud/axios#935

Merged
st3iny merged 1 commit intonextcloud:mainfrom
hereje:feat/replace-xhr-with-nextcloud-axios
Apr 10, 2025
Merged

feat!: replace xhr provider with @nextcloud/axios#935
st3iny merged 1 commit intonextcloud:mainfrom
hereje:feat/replace-xhr-with-nextcloud-axios

Conversation

@hereje
Copy link
Copy Markdown
Contributor

@hereje hereje commented Mar 29, 2025

  • replace xml http request with @nextcloud/axios in request.js
  • reorganize code in class Request so that it bevahes as with xhr
  • update test suite requestTest.js

Fixes: #718

@hereje hereje force-pushed the feat/replace-xhr-with-nextcloud-axios branch from f8dba05 to 071f4a5 Compare March 30, 2025 03:56
@hereje hereje marked this pull request as ready for review March 30, 2025 15:09
@hereje hereje requested review from hamza221 and st3iny as code owners March 30, 2025 15:09
Copy link
Copy Markdown
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Awesome, thanks! I will take a look at the code as soon as I can.

API-wise this is a breaking change. The xhrProvider argument is exposed publicly via DavClient. Removing it is a breaking change and we should do a major bump. I'm fine with that.

Please also remove the xhrProvider argument from DavClient's constructor in src/index.js in line 33.

@st3iny st3iny force-pushed the feat/replace-xhr-with-nextcloud-axios branch from b8996f0 to a65cfa7 Compare April 10, 2025 07:55
@st3iny
Copy link
Copy Markdown
Member

st3iny commented Apr 10, 2025

I rebased to fix conflicts.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 10, 2025

Codecov Report

Attention: Patch coverage is 98.21429% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.14%. Comparing base (62ebc66) to head (23758d9).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/index.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #935      +/-   ##
==========================================
+ Coverage   83.90%   85.14%   +1.23%     
==========================================
  Files          30       30              
  Lines        1255     1245      -10     
  Branches      204      206       +2     
==========================================
+ Hits         1053     1060       +7     
+ Misses        202      185      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- replace xml http request with @nextcloud/axios
in request.js
- reorganize code in class Request so that it
bevahes as with xhr
- update test suite requestTest.js

Co-authored-by: Richard Steinmetz <richard@steinmetz.cloud>
Signed-off-by: Angel Mendez Cano <amendez1988@gmail.com>
@st3iny st3iny force-pushed the feat/replace-xhr-with-nextcloud-axios branch from a65cfa7 to 23758d9 Compare April 10, 2025 13:13
@st3iny st3iny changed the title feat: replace xhr provider with @nextcloud/axios feat!: replace xhr provider with @nextcloud/axios Apr 10, 2025
Copy link
Copy Markdown
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. Thank you very much for your contribution!

I adjusted some minor things, implemented abort signal handling and improved the testing of it.

@st3iny st3iny merged commit 6304576 into nextcloud:main Apr 10, 2025
10 checks passed
@hereje hereje deleted the feat/replace-xhr-with-nextcloud-axios branch April 10, 2025 14:07
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.

Migrate Request class to @nextcloud/axios

2 participants