-
Notifications
You must be signed in to change notification settings - Fork 539
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
base: main
Are you sure you want to change the base?
Conversation
@@ -3,7 +3,6 @@ | |||
* Licensed under the MIT License. | |||
*/ | |||
|
|||
import { performance } from "@fluidframework/common-utils"; |
There was a problem hiding this comment.
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).
"@fluidframework/gitresources": "workspace:~", | ||
"@fluidframework/protocol-definitions": "^3.2.0", | ||
"@types/events": "^3.0.0", |
There was a problem hiding this comment.
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.
export { | ||
TypedEventEmitter, | ||
TypedEventTransform, | ||
IEvent, | ||
IEventProvider, | ||
IEventTransformer, | ||
TransformedEvent, | ||
EventEmitterEventType, | ||
IEventThisPlaceHolder, | ||
ReplaceIEventThisPlaceHolder, | ||
} from "./typedEventEmitter"; |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
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. |
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? |
Description
This is an experiment around removing the common-utils dependency in the server/routerlicious release group.
There are 3 major "approaches":
@fluidframework/server-services-core
since almost everything else in the server group depends on it.@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.Buffer
, encoding (which leveragesBuffer
), 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 theevents
package and a pattern we used already to import from it when aliased asevents_pkg
to avoid confusion with Node's ownevents
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.