Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Feb 8, 2024

Problem:

Current way we handle authorize call is prone to race hazards.

Solution for problem:

  • switched from useQuery to useMutation for authorize call
  • added AuthProvider to keep common state between components and hooks

Subsequent problem:

Above solution require AuthProvider to be added. Which is trivial for wallets and -v2 packages.
Unfortunatelly, against its purpose, /api is actually used in legacy code and even /hooks package. Which is not trivial to separate now.
Also, I have not found acceptable backwards-compatybile method.
Meaning, changes to improve race hazards in wallets would break legacy code :(

Subsequent solution:

Ideally, we should have semver versioning. Given we do not have it at the moment, as an baby step into that direction, introduce /api-v2 package. Block /api from updates to avoid divergent copies of api. Everyone can migrate on its own time (for -v2 migration is trivial, just add , for legacy code is not trivial but also should never be used there to begin with and even current /api might just not work with legacy code whatsover)

Copy link

@review-deriv review-deriv left a comment

Choose a reason for hiding this comment

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

PR description is too short. You can use this format:

# Description
This is a summary of the changes. This should describe why do we need this change.
Whys and Hows come here.

# Cards
* https://clickup.com/12345
* https://clickup.com/12346

@vercel
Copy link

vercel bot commented Feb 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
deriv-app ✅ Ready (Inspect) Visit Preview Feb 12, 2024 10:33am

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2024

A production App ID was automatically generated for this PR. (log)

Click here to copy & paste above information.
- **PR**: [https://github.com/binary-com/deriv-app/pull/13469](https://github.com/binary-com/deriv-app/pull/13469)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-wojciech-deriv-feature-perf-3.binary.sx?qa_server=red.derivws.com&app_id=24195
    - **Original**: https://deriv-app-git-fork-wojciech-deriv-feature-perf-3.binary.sx
- **App ID**: `24195`

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2024

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 29
🟧 Accessibility 89
🟢 Best practices 92
🟧 SEO 85
🟧 PWA 78

Lighthouse ran with https://deriv-app-git-fork-wojciech-deriv-feature-perf-3.binary.sx/

@coveralls
Copy link

coveralls commented Feb 8, 2024

Coverage Status

coverage: 35.649% (-0.4%) from 36.005%
when pulling 260cd9a on wojciech-deriv:feature/perf-3
into eb60a44 on binary-com:master.

@ghost ghost temporarily deployed to Preview February 12, 2024 10:05 — with GitHub Actions Inactive
@ghost ghost temporarily deployed to Preview February 12, 2024 10:05 — with GitHub Actions Inactive
@sonarqubecloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
45 New issues

Measures
0 Security Hotspots
No data about Coverage
11.5% Duplication on New Code

See analysis details on SonarCloud

@ghost ghost changed the title [DRAFT] just draft [PROPOSAL] changes in authorisation required for wallets Feb 13, 2024
@ghost ghost closed this Feb 14, 2024
This pull request was closed.
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