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.

Sign up for GitHub

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

Jump to bottom

refactor: Remove promise.await from apps #28612

Merged
merged 7 commits into from Mar 28, 2023
Merged

refactor: Remove promise.await from apps #28612

merged 7 commits into from Mar 28, 2023

Conversation

MarcosSpessatto
Copy link
Contributor

Proposed changes (including videos or screenshots)

Jira:
ARCH-382
ARCH-383
ARCH-384
ARCH-385

Issue(s)

Steps to test or reproduce

Further comments

@MarcosSpessatto MarcosSpessatto self-assigned this Mar 24, 2023
@MarcosSpessatto MarcosSpessatto requested review from a team as code owners March 24, 2023 18:07
@codecov
Copy link

codecov bot commented Mar 24, 2023 •
edited

Codecov Report

Merging #28612 (5ceaa3a) into develop (729b7cc) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #28612      +/-   ##
===========================================
- Coverage    44.99%   44.98%   -0.02%     
===========================================
  Files          753      753              
  Lines        14667    14667              
  Branches      2130     2130              
===========================================
- Hits          6600     6598       -2     
- Misses        7763     7764       +1     
- Partials       304      305       +1
Flag Coverage Δ
e2e 44.95% <0.00%> (-0.01%) ⬇️

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

sampaiodiego
sampaiodiego approved these changes Mar 24, 2023
View reviewed changes
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

@RocketChat/apps can you please help confirm this is not creating a breaking change?

@ggazzo
Copy link
Member

ggazzo commented Mar 28, 2023

these are the only methods we can change to async :
deleteUsersCreatedByApp
AppLivechatBridge -> isOnline

@d-gubert
Copy link
Member

d-gubert commented Mar 28, 2023 •
edited

@RocketChat/apps can you please help confirm this is not creating a breaking change?

#27024 was doing something similar, there's a couple things to note:

EDIT (previous intent wasn't clear)

The mentioned changes should maybe be added to this PR as well. Or do you think that we should simply keep it in the radar for the future? We might need to break down the previously mentioned PR (which we had already started doing at #28508) and make those changes incrementally

@ggazzo
Copy link
Member

ggazzo commented Mar 28, 2023

what does that mean? that this pr is useless or that we should keep it?
I don't think the PR you mentioned is in a healthy state enough to delivered (too many changes, too many conflicts).

If you can guarantee that this pr is good, providing approval or requesting changes, we can continue with the plan.

@ggazzo
Copy link
Member

ggazzo commented Mar 28, 2023

Since we are following this idea of doing changes small as we can and keep everything working I cant see any problem on keeping the pointed changes in mind and apply them later

Or do you think that we should simply keep it in the radar for the future?

@ggazzo ggazzo merged commit 258d82e into develop Mar 28, 2023
35 checks passed
@ggazzo ggazzo deleted the refactor/promise-await-apps-1 branch March 28, 2023 18:51
gabriellsh added a commit that referenced this pull request Mar 30, 2023
@gabriellsh
Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into feat…
4a1d5ba 
…/federation-public-room-search-2

* 'develop' of github.com:RocketChat/Rocket.Chat: (186 commits)
  fix: Unauthorized Toast Message when accessing a channel's team that user are not a member (#28670)
  regression: fix apps-engine persistence methods (#28688)
  chore: Update `codeql` to v2 (#28692)
  fix: Avoid `useEffect` loop (#28699)
  refactor: Remove `canned responses` model (#28686)
  refactor(models): Use Messages Raw model (9/N) (#28693)
  chore: Delete unused `.../surfaces/SurfaceContext.tsx` (#28690)
  i18n: fix sentence in 2fa email modal text (#28683)
  fix: change default text color in homepage custom content block (#28685)
  refactor(models): Use Messages Raw model (8/N) (#28678)
  refactor: move hasAllPermission to hasAllPermissionAsync (#28674)
  refactor: Feature parity for EE models (#28614)
  refactor(client): Dead code and spurious exports (#28654)
  fix(client): Fuselage's icon font reloads too often (#28673)
  refactor: Subscriptions model - 5 (#28550)
  regression: Error inserting read-receipts with the new Messages model (#28671)
  refactor: `LivechatDepartments` - 1/2 (#28664)
  chore: Update Apps-Engine to latest (#28646)
  regression: Room Not Found bg color (#28668)
  refactor: Remove promise.await from apps (#28612)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sampaiodiego sampaiodiego sampaiodiego approved these changes

Assignees

@MarcosSpessatto MarcosSpessatto

Labels
None yet
Projects
Fibers Removal
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MarcosSpessatto @ggazzo @d-gubert @sampaiodiego