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

#1509 fetch() implementation #5372

Merged
merged 64 commits into from
Nov 8, 2023
Merged

#1509 fetch() implementation #5372

merged 64 commits into from
Nov 8, 2023

Conversation

rrrooommmaaa
Copy link
Contributor

@rrrooommmaaa rrrooommmaaa commented Aug 20, 2023

This PR replaces JQuery ajax calls with fetch()
Modifies the mock API to use HTTP/2 -- required to display fetch upload progress.

issue #1509


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@rrrooommmaaa rrrooommmaaa marked this pull request as draft August 20, 2023 16:28
@rrrooommmaaa rrrooommmaaa mentioned this pull request Aug 20, 2023
5 tasks
@sosnovsky sosnovsky marked this pull request as ready for review November 6, 2023 21:17
@sosnovsky sosnovsky requested a review from ioanmo226 November 6, 2023 21:17
@sosnovsky
Copy link
Collaborator

@ioanmo226 this issue is part of bigger refactoring to implement Manifest V3 requirements, which should be required in the future for all extensions. In this PR we switch to using fetch instead of Jquery.Ajax, as in V3 it's not possible to use XMLHTTPRequest (which is used by Jquery.Ajax).

Roman worked on this switch, but there were some issues in loading messages in Firefox, as currently it doesn't support some features of fetch. So I worked on implementing fallbacks which use XMLHTTPRequest, but can be removed and switched to fetch when all needed functionality will be implemented there.

.semaphore/semaphore.yml Outdated Show resolved Hide resolved
@sosnovsky sosnovsky requested a review from ioanmo226 November 7, 2023 09:29
Copy link
Collaborator

@ioanmo226 ioanmo226 left a comment

Choose a reason for hiding this comment

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

Works great. small comments

extension/js/common/api/email-provider/gmail/google.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
extension/js/common/api/shared/api.ts Show resolved Hide resolved
extension/js/common/api/shared/api.ts Show resolved Hide resolved
@ioanmo226
Copy link
Collaborator

ioanmo226 commented Nov 7, 2023

By the way, might better to add another reviewer as this seems breaking changes and I might have missed something?
Like @tomholub or @rrrooommmaaa ?

@sosnovsky
Copy link
Collaborator

By the way, might better to add another reviewer as this seems breaking changes and I might have missed something? Like @tomholub or @rrrooommmaaa ?

Unfortunately Roman doesn't work on FlowCrypt anymore, that's why I was finalising this PR.
Tom checked this PR before I started working on it, looked good for him. I worked mostly on fixing issues found during previous reviews, so I think it shouldn't break anything as currently all tests are passing without issues.

@ioanmo226
Copy link
Collaborator

Got it.

@ioanmo226
Copy link
Collaborator

Please check test

@sosnovsky
Copy link
Collaborator

Please check test

We've missed that Google API urls have different base urls - ${GMAIL_GOOGLE_API_HOST}/gmail/v1/users/me and ${GMAIL_GOOGLE_API_HOST}/upload/gmail/v1/users/me, so it caused test fails.
Reverted this change, tests should pass now

@ioanmo226 ioanmo226 enabled auto-merge (squash) November 8, 2023 07:30
@ioanmo226 ioanmo226 merged commit 9fa013a into master Nov 8, 2023
8 checks passed
@ioanmo226 ioanmo226 deleted the issue-1509-fetch branch November 8, 2023 07:31
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.

4 participants