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

refactor(server): Remove dependency on common-utils #23998

Draft
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

alexvy86
Copy link
Contributor

@alexvy86 alexvy86 commented Mar 6, 2025

Description

This is an experiment around removing the common-utils dependency in the server/routerlicious release group.

There are 3 major "approaches":

  • A couple of things were used in a single package or a very few places; those I just moved to live in the package that used them.
  • Some things were used in many packages, but stayed firmly on the server side, thus guaranteed to be running in a NodeJS environment. Those I moved to @fluidframework/server-services-core since almost everything else in the server group depends on it.
  • Some things were used in several packages but in particular they were used in @fluidframework/server-services-client, which the client release group depends on, and thus they could end up running in NodeJS or in the browser. Those I moved to @fluidframework/server-services-client itself, and re-exposed them from there for the rest of the packages that need them. Similarly to @fluidframework/server-services-core, @fluidframework/server-services-client is a package that almost everything else in the server group depends on.
    • In particular, things related to Buffer, encoding (which leverages Buffer), and file hashing, moved here.

One thing stood out while moving: TypedEventEmitter. It ended up copied in two different places (@fluidframework/server-sevices-core and @fluidframework/protocol-base) because neither depends on the other. It's built on top of several types which ended up having to be exportedin the 2 packages where it ended up copied. And I ran into some weirdness with the events package and a pattern we used already to import from it when aliased as events_pkg to avoid confusion with Node's own events module (import events_pkg from "events_pkg"; const { EventEmitter } = events_pkg;). Importing NodeJS' EventEmitter seems to have worked.

Reviewer Guidance

The review process is outlined on this wiki page.

@github-actions github-actions bot added area: server Server related issues (routerlicious) base: main PRs targeted against main branch labels Mar 6, 2025
@@ -3,7 +3,6 @@
* Licensed under the MIT License.
*/

import { performance } from "@fluidframework/common-utils";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Server code only runs in node, it can use the native performance.now() directly, no need to try to proxy through an isomorphic performance object (which incidentally was already removed in main for the client release group).

@github-actions github-actions bot added the public api change Changes to a public API label Mar 6, 2025
"@fluidframework/gitresources": "workspace:~",
"@fluidframework/protocol-definitions": "^3.2.0",
"@types/events": "^3.0.0",
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 this is really necessary, since it's a @types package. It's there in common-utils.

Comment on lines +8 to +18
export {
TypedEventEmitter,
TypedEventTransform,
IEvent,
IEventProvider,
IEventTransformer,
TransformedEvent,
EventEmitterEventType,
IEventThisPlaceHolder,
ReplaceIEventThisPlaceHolder,
} from "./typedEventEmitter";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the currently exported QuorumClients is built on top of TypedEventEmitter, moving the latter here means that it and all its building blocks need to be exported.

"@fluidframework/gitresources": "workspace:~",
"@fluidframework/protocol-definitions": "^3.2.0",
"@fluidframework/server-services-client": "workspace:~",
"@fluidframework/server-services-telemetry": "workspace:~",
"@types/events": "^3.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as in protocol-base; not sure it's necessary. Brought it because it's in common-utils.

@tylerbutler
Copy link
Member

I think @ChumpChief and @anthony-murphy have a pretty good idea of what the blockers are for full split of this code. Suggest syncing with them before going too far.

@ChumpChief
Copy link
Contributor

The impression I had was that common-utils was intended to be the set of things that we expected both client/server to use (as opposed to client-utils only being used by the client, for example) and intentionally shared. Is there a particular motivation for splitting it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants