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

🛠 global resolver #2023

Merged
merged 19 commits into from
Apr 14, 2022
Merged

🛠 global resolver #2023

merged 19 commits into from
Apr 14, 2022

Conversation

romanesko
Copy link
Contributor

it's a draft of global reserver

@RomaricMourgues RomaricMourgues changed the title global resolver 🛠 global resolver Mar 29, 2022
@RomaricMourgues RomaricMourgues marked this pull request as ready for review March 29, 2022 14:49
RomaricMourgues
RomaricMourgues suggested changes Mar 30, 2022
View reviewed changes
Copy link
Contributor

@RomaricMourgues RomaricMourgues left a comment

Choose a reason for hiding this comment

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

So three topics:

  • Rename the _services in config
  • Rename CoreServices in global-resolver
  • Reduce number of services in global-resolver

Also I am thinking that maybe we could have an other global-resolver for the platform services so in /core/platform/services/global-resolver.ts instead of having PlatformServices in the current global-resolver.

twake/backend/node/config/default.json Outdated
"webserver",
"websocket"
],
"_services": [
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this ? Should it be removed ? Or renamed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately we need all services here for now, after I change the way web services are detected it will be necessary to specify only the platform services here

twake/backend/node/src/services/global-resolver.ts Outdated
notificationBadge: UserNotificationBadgeServiceAPI;
notificationPreferences: UserNotificationPreferencesAPI;
messages: MessageThreadMessagesServiceAPI;
threads: MessageThreadsServiceAPI;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just import MessageServiceAPI ? It contains:

  userBookmarks: MessageUserBookmarksServiceAPI;
  threads: MessageThreadsServiceAPI;
  messages: MessageThreadMessagesServiceAPI;
  views: MessageViewsServiceAPI;
  engine: MessagesEngine;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no more MessageServiceAPI 😬
One of goals was to remove this grouping services, because of the over-use of the same services in different groups.

I told you today that I'm going to group some of them on level of global resolver for convinience

twake/backend/node/src/services/global-resolver.ts Outdated
threads: MessageThreadsServiceAPI;
userBookmarks: MessageUserBookmarksServiceAPI;
applications: MarketplaceApplicationServiceAPI;
companyApplications: CompanyApplicationServiceAPI;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, ApplicationServiceAPI contains already this MarketplaceApplicationServiceAPI and CompanyApplicationServiceAPI

twake/backend/node/src/services/global-resolver.ts
members: MemberService;
channelPendingEmail: ChannelPendingEmailService;
tab: TabService;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I think we would get better code if we have one item here for each folder we have in /services/ or /core/services/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I understand you right, but we have here one item for each folder we have in services…. no?

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't have a /services/channelPendingEmail nor a /services/threads folders, it is related to the "I told you today that I'm going to group some of them on level of global resolver for convinience"

twake/backend/node/src/services/global-resolver.ts Outdated Show resolved Hide resolved
@romanesko
Copy link
Contributor Author

I've pushed fixes, take a look

@RomaricMourgues
Copy link
Contributor

@romanesko why is this not merged yet ? The more we work on other stuff the more difficult it will be to merge right ? :p

@romanesko
Copy link
Contributor Author

@romanesko why is this not merged yet ?
I can't merge, only you can

@romanesko
Merge remote-tracking branch 'origin/develop' into global-resolver
a22bb3b 
# Conflicts:
#	twake/backend/node/src/services/applications/web/routes.ts
#	twake/backend/node/test/e2e/application/application-events.spec.ts
@romanesko
global-resolver merge issues
34fdf1c
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
4740bd5 
…al-resolver

# Conflicts:
#	twake/backend/node/src/services/channels/services/channel/service.ts
#	twake/backend/node/src/services/messages/services/messages.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
3197225 
…al-resolver

# Conflicts:
#	twake/backend/node/src/services/channels/services/channel/service.ts
#	twake/backend/node/src/services/messages/services/messages.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
eafc1e5 
…al-resolver

# Conflicts:
#	twake/backend/node/src/services/channels/services/channel/service.ts
#	twake/backend/node/src/services/messages/services/messages.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
721d39b 
…al-resolver

# Conflicts:
#	twake/backend/node/src/services/channels/services/channel/service.ts
#	twake/backend/node/src/services/messages/services/messages.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
67028a4 
…al-resolver

# Conflicts:
#	twake/backend/node/package-lock.json
#	twake/backend/node/src/services/applications/api.ts
#	twake/backend/node/src/services/applications/services/applications.ts
#	twake/backend/node/src/services/applications/services/index.ts
#	twake/backend/node/src/services/applications/web/controllers/applications.ts
#	twake/backend/node/src/services/applications/web/controllers/company-applications.ts
#	twake/backend/node/src/services/applicationsapi/index.ts
#	twake/backend/node/src/services/applicationsapi/web/controllers/index.ts
#	twake/backend/node/src/services/messages/services/engine/index.ts
#	twake/backend/node/src/services/messages/web/controllers/threads.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
ab7daec 
…al-resolver

# Conflicts:
#	twake/backend/node/package-lock.json
#	twake/backend/node/src/services/applications/api.ts
#	twake/backend/node/src/services/applications/services/applications.ts
#	twake/backend/node/src/services/applications/services/index.ts
#	twake/backend/node/src/services/applications/web/controllers/applications.ts
#	twake/backend/node/src/services/applications/web/controllers/company-applications.ts
#	twake/backend/node/src/services/applicationsapi/index.ts
#	twake/backend/node/src/services/applicationsapi/web/controllers/index.ts
#	twake/backend/node/src/services/messages/services/engine/index.ts
#	twake/backend/node/src/services/messages/web/controllers/threads.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
56c5271 
…al-resolver

# Conflicts:
#	twake/backend/node/package-lock.json
#	twake/backend/node/src/services/applications/api.ts
#	twake/backend/node/src/services/applications/services/applications.ts
#	twake/backend/node/src/services/applications/services/index.ts
#	twake/backend/node/src/services/applications/web/controllers/applications.ts
#	twake/backend/node/src/services/applications/web/controllers/company-applications.ts
#	twake/backend/node/src/services/applicationsapi/index.ts
#	twake/backend/node/src/services/applicationsapi/web/controllers/index.ts
#	twake/backend/node/src/services/messages/services/engine/index.ts
#	twake/backend/node/src/services/messages/web/controllers/threads.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
8752f73 
…al-resolver

# Conflicts:
#	twake/backend/node/package-lock.json
#	twake/backend/node/src/services/applications/api.ts
#	twake/backend/node/src/services/applications/services/applications.ts
#	twake/backend/node/src/services/applications/services/index.ts
#	twake/backend/node/src/services/applications/web/controllers/applications.ts
#	twake/backend/node/src/services/applications/web/controllers/company-applications.ts
#	twake/backend/node/src/services/applicationsapi/index.ts
#	twake/backend/node/src/services/applicationsapi/web/controllers/index.ts
#	twake/backend/node/src/services/messages/services/engine/index.ts
#	twake/backend/node/src/services/messages/web/controllers/threads.ts
@romanesko
Merge branch 'develop' of https://github.com/linagora/Twake into glob…
803f0a2 
…al-resolver

# Conflicts:
#	twake/backend/node/package-lock.json
#	twake/backend/node/src/services/applications/api.ts
#	twake/backend/node/src/services/applications/services/applications.ts
#	twake/backend/node/src/services/applications/services/index.ts
#	twake/backend/node/src/services/applications/web/controllers/applications.ts
#	twake/backend/node/src/services/applications/web/controllers/company-applications.ts
#	twake/backend/node/src/services/applicationsapi/index.ts
#	twake/backend/node/src/services/applicationsapi/web/controllers/index.ts
#	twake/backend/node/src/services/messages/services/engine/index.ts
#	twake/backend/node/src/services/messages/web/controllers/threads.ts
@romanesko
Merge branch 'develop' of github.com:romanesko/Twake into global-reso…
dc84425 
…lver

# Conflicts:
#	twake/backend/node/src/services/applications/web/controllers/applications.ts
#	twake/backend/node/src/services/channels/services/channel/service.ts
@RomaricMourgues RomaricMourgues merged commit 564681b into linagora:develop Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RomaricMourgues RomaricMourgues RomaricMourgues requested changes

Assignees
No one assigned
Labels
staging:main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@romanesko @RomaricMourgues @shepilov