Move events library to core-interfaces and core-utils #23037

sonalideshpandemsft · 2024-11-08T19:16:16Z

This change relocates event library from SharedTree to core-utils and core-interfaces. It updates imports in both the tree and presence packages to reference the event library.

ADO

packages/common/core-interfaces/api-report/core-interfaces.beta.api.md

msfluid-bot

Code Coverage Summary

No packages impacted by the change.

Baseline commit: 3fe3fa5
Baseline build: 306842
Happy Coding!!

Code coverage comparison check passed!!

sonalideshpandemsft · 2024-11-12T19:47:47Z

packages/common/core-utils/src/events/emitter.ts

@@ -168,7 +171,7 @@ export class EventEmitter<TListeners extends Listeners<TListeners>>
 			const eventDescription =
 				typeof eventName === "symbol" ? eventName.description : String(eventName.toString());

-			throw new UsageError(
+			throw new Error(


UsageError has been replaced with the standard Error type because telemetry-utils cannot be added as a dependency in core-utils, due to a layer-check failure.

fyi: @noencke

sonalideshpandemsft · 2024-11-12T19:48:39Z

packages/common/core-utils/src/test/events/events.spec.ts

@@ -5,7 +5,7 @@

 import { strict as assert } from "node:assert";

-import { validateAssertionError } from "@fluidframework/test-runtime-utils/internal";
+// import { validateAssertionError } from "@fluidframework/test-runtime-utils/internal";


validateAssertionError has been replaced with the standard Error type because test-runtime-utils cannot be added as a dependency in core-utils, due to a layer-check failure.

fyi, @noencke

msfluid-bot · 2024-11-14T14:06:35Z

⯅ @fluid-example/bundle-size-tests: +391 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	465.72 KB	465.75 KB	⯅ +35 Bytes
azureClient.js	563.04 KB	563.09 KB	⯅ +49 Bytes
connectionState.js	724 Bytes	724 Bytes	■ No change
containerRuntime.js	262.34 KB	262.35 KB	⯅ +14 Bytes
fluidFramework.js	426.99 KB	427.08 KB	⯅ +87 Bytes
loader.js	134.18 KB	134.19 KB	⯅ +14 Bytes
map.js	42.71 KB	42.71 KB	⯅ +7 Bytes
matrix.js	149.84 KB	149.85 KB	⯅ +7 Bytes
odspClient.js	528.83 KB	528.88 KB	⯅ +49 Bytes
odspDriver.js	97.88 KB	97.9 KB	⯅ +21 Bytes
odspPrefetchSnapshot.js	42.81 KB	42.83 KB	⯅ +14 Bytes
sharedString.js	165.77 KB	165.78 KB	⯅ +7 Bytes
sharedTree.js	417.45 KB	417.53 KB	⯅ +80 Bytes
Total Size	3.37 MB	3.37 MB	⯅ +391 Bytes

Baseline commit: 3fe3fa5

Generated by 🚫 dangerJS against 3ec50b1

anthony-murphy · 2024-11-14T17:36:06Z

packages/dds/tree/src/util/typeUtils.ts

+ * @system
+ * @public
+ */
+export type UnionToIntersection<T> = (T extends T ? (k: T) => unknown : never) extends (


nit. this file doesn't really need to be edited. if you keep your local main branch up to date you can run git checkout main -- packages/dds/tree/src/util/typeUtils.ts to clobber it with the version from main, kind of like a revert. you can even do it on whole sub-paths if you want to undo changes in an area.

packages/dds/tree/package.json

alexvy86

I haven't looked at the whole PR, but wondering if client-utils a better fit to move eventing to (core-utils says it's for fluid-agnostic utilities). If the whole set of features we're moving don't have dependencies on Fluid-specific things, then core-utils might be fine, but the fact that core-utils now requires a new dependency on core-interfaces (which contains Fluid-specific types) makes me think that's not the case and that client-utils might indeed be a better target.

Also, we'll want a changeset for this PR.

alexvy86 · 2024-11-14T17:45:50Z

packages/common/core-interfaces/src/events/listeners.ts

@@ -4,7 +4,7 @@
 */

 /**
- * `true` iff the given type is an acceptable shape for a {@link Listeners | event} listener
+ * `true` if the given type is an acceptable shape for a {@link Listeners | event} listener


iff might have been a deliberate abbreviation of "if and only if" but I think in this context it doesn't make a difference.

packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts

alexvy86 · 2024-11-14T17:58:33Z

packages/dds/tree/src/util/nestedSet.ts

@@ -3,7 +3,8 @@
 * Licensed under the MIT License.
 */

-import { type NestedMap, getOrDefaultInNestedMap, setInNestedMap } from "./nestedMap.js";
+import type { NestedMap } from "@fluidframework/core-interfaces/internal";
+import { getOrDefaultInNestedMap, setInNestedMap } from "./nestedMap.js";


With NestedMap moving to a common place, I find it a bit weird that some of the functions to use it are in tree and not in core-utils. Should we move the functions there?

I had this same concern but IMO this is too much scope growth. I'm inclined to duplicate what's needed for emitter.ts into client-utils and change none of the utility functions' homes in this PR.

At a glance I'm not a fan of this nested map concept and I think it needs to defend itself in its own PR.

alexvy86 · 2024-11-14T18:00:09Z

packages/framework/presence/src/latestValueManager.ts

@@ -17,7 +19,6 @@ import type {
 	JsonSerializable,
 } from "@fluid-experimental/presence/internal/core-interfaces";
 import type { ISubscribable } from "@fluid-experimental/presence/internal/events";
-import { createEmitter } from "@fluid-experimental/presence/internal/events";


Tangent: I'm a bit confused here. Presence does all its imports with the full package name instead of relative paths? (cc @jason-ha).

core-utils also does: https://github.com/microsoft/FluidFramework/blob/main/packages/common/core-utils/src/test/timer.spec.ts#L12. I think that's to avoid adding eslint suppression.

It isn't lint suppression.
The best use for "self-references" is in testing. We should test against the public API more. For that we should use package reference where possible. Then we can also see where we are violating API to do certain testing.
Here it is used to manage the temp duplication of the tree events package and workaround api-extractor fussiness. Everything in presence that is using one these complex presence/internal/foo paths has a plan to get rid of them.

sonalideshpandemsft · 2024-11-14T18:39:07Z

I haven't looked at the whole PR, but wondering if client-utils a better fit to move eventing to (core-utils says it's for fluid-agnostic utilities). If the whole set of features we're moving don't have dependencies on Fluid-specific things, then core-utils might be fine, but the fact that core-utils now requires a new dependency on core-interfaces (which contains Fluid-specific types) makes me think that's not the case and that client-utils might indeed be a better target.

Also, we'll want a changeset for this PR.

That's a good point. I thought about adding events to client-utils but the README suggests it's a "dumping ground," so it’s best to avoid adding new functionality in there? Probably it's okay to have core-interfaces as a dependency on core-utils and core-utils not be fluid agnostic utility package?

anthony-murphy · 2024-11-14T18:49:15Z

I haven't looked at the whole PR, but wondering if client-utils a better fit to move eventing to (core-utils says it's for fluid-agnostic utilities). If the whole set of features we're moving don't have dependencies on Fluid-specific things, then core-utils might be fine, but the fact that core-utils now requires a new dependency on core-interfaces (which contains Fluid-specific types) makes me think that's not the case and that client-utils might indeed be a better target.
Also, we'll want a changeset for this PR.

That's a good point. I thought about adding events to client-utils but the README suggests it's a "dumping ground," so it’s best to avoid adding new functionality in there? Probably it's okay to have core-interfaces as a dependency on core-utils and core-utils not be fluid agnostic utility package?

client-utils is better, but it is fluid-internal :( , so we shouldn't put non-internal things there

alexvy86 · 2024-11-14T18:51:42Z

Probably it's okay to have core-interfaces as a dependency on core-utils and core-utils not be fluid agnostic utility package?

I don't feel too strongly against this, but it seems to me like a good office hours topic. I'm also not sure what I think about client-utils own README stating "In most cases, the code would be better placed in a
package with a clear identity (e.g. an "events" package for shared event infrastructure)". That sounds like it could lead to even more packages, which we also want to avoid. Well-defined modules within client-utils seems like a better approach to me. But again, probably worth bringing up in office hours.

alexvy86 · 2024-11-14T18:55:49Z

client-utils is better, but it is fluid-internal :( , so we shouldn't put non-internal things there

Yeah, that's probably one of the strongest arguments against it. I think technically nothing prevents us from exposing APIs there as @public and re-exporting them from the appropriate public-facing packages, but that sounds like asking for trouble or complicated rules about how to deal with those APIs. So maybe rebranding core-utils as not necessarily being dependency-free is the way to go.

jason-ha · 2024-11-14T19:02:17Z

-- Tangent from the client-utils discussion thread --

I haven't looked at the whole PR, but wondering if client-utils a better fit to move eventing to (core-utils says it's for fluid-agnostic utilities). If the whole set of features we're moving don't have dependencies on Fluid-specific things, then core-utils might be fine, but the fact that core-utils now requires a new dependency on core-interfaces (which contains Fluid-specific types) makes me think that's not the case and that client-utils might indeed be a better target.

Also, we'll want a changeset for this PR.

Eventing is fluid agnostic, and I hope remains fluid agnostic. So, I agree we should not mix with fluid dependencies. We also already have eventing support in core-utils. This is part of the transition to replace that.

So, this is perhaps an argument against the suggestion to put the interfaces in core-interfaces.
Either we let the types remain in core-utils (my preference) OR we create a core-utils-interfaces.

anthony-murphy · 2024-11-14T19:18:11Z

-- Tangent from the client-utils discussion thread --

I haven't looked at the whole PR, but wondering if client-utils a better fit to move eventing to (core-utils says it's for fluid-agnostic utilities). If the whole set of features we're moving don't have dependencies on Fluid-specific things, then core-utils might be fine, but the fact that core-utils now requires a new dependency on core-interfaces (which contains Fluid-specific types) makes me think that's not the case and that client-utils might indeed be a better target.
Also, we'll want a changeset for this PR.

Eventing is fluid agnostic, and I hope remains fluid agnostic. So, I agree we should not mix with fluid dependencies. We also already have eventing support in core-utils. This is part of the transition to replace that.

So, this is perhaps an argument against the suggestion to put the interfaces in core-interfaces. Either we let the types remain in core-utils (my preference) OR we create a core-utils-interfaces.

i don't think we want or should be in the business of creating reusable libraries which are external to fluid, as it's not a support burden we should take on. For the existing eventing it is split between core-interfaces and client-utils, so maybe we just ignore the odd scoping, and go with client-utils anyway.

jason-ha · 2024-11-14T19:26:20Z

i don't think we want or should be in the business of creating reusable libraries which are external to fluid, as it's not a support burden we should take on. For the existing eventing it is split between core-interfaces and client-utils, so maybe we just ignore the odd scoping, and go with client-utils anyway.

Seems very funny that eventing has types in core-interfaces but core-utils that provides implementation doesn't reference core-interfaces. .... well that is because core-utils doesn't provide implementation (I made bad assumption above). client-utils is providing it. I don't see reason why the implementation shouldn't be in client-utils.

github-actions · 2024-11-15T19:42:34Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  444350 links
    3418 destination URLs
       2 URLs ignored
       0 warnings
       0 errors

packages/common/client-utils/src/indexBrowser.ts

packages/dds/tree/src/index.ts

sonalideshpandemsft · 2024-11-19T10:25:53Z

I’ve opened a new PR that addresses most of the feedback from this one: #23141

This change relocates event library from SharedTree to client-utils and core-interfaces. Old PR: #23037 [ADO#6595](https://dev.azure.com/fluidframework/internal/_workitems/edit/6595)

sonalideshpandemsft added 4 commits November 7, 2024 23:05

wip

8824e23

comment tests and build core-interface

5c5c343

wip

3375d11

fix lock file

9bf973a

github-actions bot added area: dds Issues related to distributed data structures area: dds: tree public api change Changes to a public API base: main PRs targeted against main branch labels Nov 8, 2024

anthony-murphy reviewed Nov 8, 2024

View reviewed changes

packages/common/core-interfaces/api-report/core-interfaces.beta.api.md Outdated Show resolved Hide resolved

anthony-murphy reviewed Nov 8, 2024

View reviewed changes

packages/common/core-interfaces/api-report/core-interfaces.beta.api.md Outdated Show resolved Hide resolved

wip

d2e6bc1

github-actions bot added area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct dependencies Pull requests that update a dependency file labels Nov 12, 2024

sonalideshpandemsft added 3 commits November 12, 2024 13:19

Merge remote-tracking branch 'origin' into mv-events-library

d6fa82e

wip

c03bcb5

move events to core-utils

02d5f2f

github-actions bot removed the dependencies Pull requests that update a dependency file label Nov 12, 2024

sonalideshpandemsft changed the title ~~Move events library to core-interfaces~~ Move events library to core-utils Nov 12, 2024

Delete interop.ts as it's unused

b15eb27

msfluid-bot reviewed Nov 12, 2024

View reviewed changes

fix CI failure

43fbee8

sonalideshpandemsft force-pushed the mv-events-library branch from e750cb1 to 6bdacc5 Compare November 12, 2024 18:35

mark eventemitter as internal

b4ea361

sonalideshpandemsft force-pushed the mv-events-library branch from 6bdacc5 to b4ea361 Compare November 12, 2024 18:41

update imports in presence package

fa229a3

sonalideshpandemsft force-pushed the mv-events-library branch from 21b1057 to fa229a3 Compare November 12, 2024 19:13

sonalideshpandemsft added 2 commits November 12, 2024 19:44

fix eslint errors

744cef7

Merge branch 'main' into mv-events-library

512667e

sonalideshpandemsft commented Nov 12, 2024

View reviewed changes

nits

50a415f

sonalideshpandemsft force-pushed the mv-events-library branch from f0d55d8 to b84034c Compare November 14, 2024 13:12

sonalideshpandemsft changed the title ~~Move events library to core-utils~~ Move events library to core-interfaces and core-utils Nov 14, 2024

sonalideshpandemsft marked this pull request as ready for review November 14, 2024 17:28

sonalideshpandemsft requested review from a team as code owners November 14, 2024 17:28

split tests

3ec50b1

sonalideshpandemsft force-pushed the mv-events-library branch from e27d63e to 3ec50b1 Compare November 14, 2024 17:33

anthony-murphy reviewed Nov 14, 2024

View reviewed changes

packages/dds/tree/package.json Outdated Show resolved Hide resolved

alexvy86 reviewed Nov 14, 2024

View reviewed changes

move impl to client-utils

9f560f9

sonalideshpandemsft force-pushed the mv-events-library branch from 6b9eb93 to 9f560f9 Compare November 15, 2024 18:33

sonalideshpandemsft added 2 commits November 15, 2024 18:35

Merge remote-tracking branch 'origin' into mv-events-library

135412a

Merge remote-tracking branch 'origin' into mv-events-library

9d254e9

ChumpChief reviewed Nov 18, 2024

View reviewed changes

packages/common/client-utils/src/indexBrowser.ts Show resolved Hide resolved

packages/dds/tree/src/index.ts Show resolved Hide resolved

sonalideshpandemsft mentioned this pull request Nov 19, 2024

Move events library to core-interfaces and client-utils #23141

Merged

sonalideshpandemsft marked this pull request as draft November 19, 2024 10:23

sonalideshpandemsft closed this Nov 21, 2024

sonalideshpandemsft deleted the mv-events-library branch November 21, 2024 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move events library to core-interfaces and core-utils #23037

Move events library to core-interfaces and core-utils #23037

sonalideshpandemsft commented Nov 8, 2024 •

edited

Loading

msfluid-bot left a comment •

edited

Loading

sonalideshpandemsft Nov 12, 2024

sonalideshpandemsft Nov 12, 2024

msfluid-bot commented Nov 14, 2024 •

edited

Loading

anthony-murphy Nov 14, 2024

alexvy86 left a comment

alexvy86 Nov 14, 2024

alexvy86 Nov 14, 2024

ChumpChief Nov 18, 2024

alexvy86 Nov 14, 2024

sonalideshpandemsft Nov 14, 2024

jason-ha Nov 14, 2024

sonalideshpandemsft commented Nov 14, 2024

anthony-murphy commented Nov 14, 2024

alexvy86 commented Nov 14, 2024

alexvy86 commented Nov 14, 2024

jason-ha commented Nov 14, 2024

anthony-murphy commented Nov 14, 2024

jason-ha commented Nov 14, 2024

github-actions bot commented Nov 15, 2024

sonalideshpandemsft commented Nov 19, 2024

Move events library to core-interfaces and core-utils #23037

Move events library to core-interfaces and core-utils #23037

Conversation

sonalideshpandemsft commented Nov 8, 2024 • edited Loading

msfluid-bot left a comment • edited Loading

Choose a reason for hiding this comment

Code Coverage Summary

Code coverage comparison check passed!!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msfluid-bot commented Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

alexvy86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonalideshpandemsft commented Nov 14, 2024

anthony-murphy commented Nov 14, 2024

alexvy86 commented Nov 14, 2024

alexvy86 commented Nov 14, 2024

jason-ha commented Nov 14, 2024

anthony-murphy commented Nov 14, 2024

jason-ha commented Nov 14, 2024

github-actions bot commented Nov 15, 2024

linkcheck output

sonalideshpandemsft commented Nov 19, 2024

sonalideshpandemsft commented Nov 8, 2024 •

edited

Loading

msfluid-bot left a comment •

edited

Loading

msfluid-bot commented Nov 14, 2024 •

edited

Loading