Move events library to core-interfaces and client-utils #23141
Conversation
github-actions
bot
added
area: dds
sonalideshpandemsft
force-pushed
the
mv-events-lib-1
branch
from
b68f4ab
to
adbec11
Compare
sonalideshpandemsft
changed the title
There was a problem hiding this comment.
Code Coverage Summary
↑ packages.dds.tree.src.core.tree:
Line Coverage Change: 0.01% Branch Coverage Change: No change
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 96.56% | 96.56% | → No change |
| Line Coverage | 88.09% | 88.10% | ↑ 0.01% |
Baseline commit: f1c1c31
Baseline build: 309286
Happy Coding!!
Code coverage comparison check passed!!
⯅ @fluid-example/bundle-size-tests: +391 Bytes
Baseline commit: f1c1c31 |
There was a problem hiding this comment.
Copilot reviewed 30 out of 43 changed files in this pull request and generated no suggestions.
Files not reviewed (13)
- packages/common/core-interfaces/api-report/core-interfaces.legacy.public.api.md: Evaluated as low risk
- packages/common/core-interfaces/src/events/emitter.ts: Evaluated as low risk
- packages/common/core-interfaces/src/events.ts: Evaluated as low risk
- packages/common/core-interfaces/src/events/README.md: Evaluated as low risk
- packages/common/core-interfaces/src/index.ts: Evaluated as low risk
- packages/dds/tree/api-report/tree.legacy.alpha.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.beta.api.md: Evaluated as low risk
- packages/dds/tree/api-report/tree.alpha.api.md: Evaluated as low risk
- packages/common/client-utils/src/indexBrowser.ts: Evaluated as low risk
- packages/common/client-utils/src/indexNode.ts: Evaluated as low risk
- packages/common/client-utils/src/events/emitter.ts: Evaluated as low risk
- packages/common/core-interfaces/src/events/index.ts: Evaluated as low risk
- packages/common/core-interfaces/api-report/core-interfaces.public.api.md: Evaluated as low risk
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
| * A class can delegate handling {@link Listenable} to the returned value while using it to emit the events. | ||
| * See also {@link EventEmitter} which be used as a base class to implement {@link Listenable} via extension. | ||
| * A class can delegate handling {@link @fluidframework/core-interfaces#Listenable} to the returned value while using it to emit the events. | ||
| * See also CustomEventEmitter which be used as a base class to implement {@link @fluidframework/core-interfaces#Listenable} via extension. |
There was a problem hiding this comment.
Nit
| * See also CustomEventEmitter which be used as a base class to implement {@link @fluidframework/core-interfaces#Listenable} via extension. | |
| * @see {@link CustomEventEmitter}, which be used as a base class to implement {@link @fluidframework/core-interfaces#Listenable} via extension. |
There was a problem hiding this comment.
This causes build failures, possibly because it isn't exported from the other index.ts. However, the @link tag works fine for tests.
| ): ReturnType<TListeners[K]>[]; | ||
| interface MapGetSet<K, V> { | ||
| get(key: K): V | undefined; | ||
| set(key: K, value: V): void; |
There was a problem hiding this comment.
Maybe more of a question for the tree folks (and not something that should change in this PR), but I'm wondering why not just use something like:
export type MapGetSet<K, V> = Pick<Map<K, V>, "get" | "set">I see that set has a void return instead of this but not sure if that is intentional and/or necessary here?
There was a problem hiding this comment.
Good question. I'm not sure how conscious of a decision it was originally, but having the return type of set be looser means it's a wee bit easier to have an object satisfy MapGetSet. I think the Tree package has some objects which are not JS maps, but which satisfy MapGetSet, and can therefore be used with e.g. getOrCreate. At best it'd be annoying to have to change their set method return this in order to satisfy the interface, and at worst it would be prohibitive because the return value is already being used for something else. I think it's a good call considering that I rarely (have I ever?) seen use of the fluent-style code that would leverage the returned this - e.g. map.set(...).size or something like that - so nothing is really lost.
You made me wonder if we could still make it more closely derived:
interface MapGetSet<K, V> extends Pick<Map<K, V>, "get"> {
set(...args: Parameters<Map<K,V>["set"]>): void
}The "typescript enjoyer and purist" part of me thinks that is cool and good, whereas the rational part of me thinks that it is less readable and will bother subsequent readers more than help them :)
| @@ -305,6 +308,17 @@ export interface ITelemetryBaseProperties { | |||
| [index: string]: TelemetryBaseEventPropertyType | Tagged<TelemetryBaseEventPropertyType>; | |||
| } | |||
|
|
|||
| // @public @sealed | |||
There was a problem hiding this comment.
Should other interfaces be marked as @sealed too?
There was a problem hiding this comment.
Do we want to prevent further extensions of this interface to ensure a fixed structure?
There was a problem hiding this comment.
Sorry, I see that this is the only @public interface - I was looking at the @internal ones too (e.g. IEmitter) which I think would be nice to declare @sealed but probably matters a lot less if they're not available to customers.
| type Listenable, | ||
| type Off, | ||
| } from "./events/index.js"; | ||
| export type { |
There was a problem hiding this comment.
Long-term, we shouldn't re-export from here (folks should import directly from the appropriate package). Is this just to avoid a breaking change for the time being? Would be good to have a plan in place to deprecate/remove these exports from this package.
There was a problem hiding this comment.
Yes, it is just to avoid breaking changes. I'll open a follow-up work item to remove them
There was a problem hiding this comment.
Removing these will be a breaking change on public API surface so it'll have to wait until 3.0, I think (?) but we'll have to deprecate them somehow before that, no? Should we do something like importing them as Listeners_base or similar in this package, and exporting them from here as a new type with its own docs and deprecation notice? I.e. something like (double check it, not sure it works as written):
import { Listeners as Listeners_base } from "@fluidframework/core-interfaces";
/**
* @inheritDoc <something here, I don't recall the syntax>
* @ deprecated This type has moved to `@fluidframework/core-interfaces` and will be removed from here in a future release.
* If you need to reference it directly, do it from `@fluidframework/core-interfaces`.
*/
export type Listeners = Listeners_base;
There was a problem hiding this comment.
I believe this is not considered breaking since it's re-exported from here as well, and there are no typetest breaks in tree.
There was a problem hiding this comment.
Right, it's not breaking now, but when we remove them from here it will be breaking, and before we can do that they should be marked as deprecated here. So I'm suggesting we deprecate them here now (while keeping them not-deprecated in core-interfaces).
There was a problem hiding this comment.
ADO: https://dev.azure.com/fluidframework/internal/_workitems/edit/25440
Another concern could be aliasing in core-interfaces: sonalideshpandemsft#41.
Would that be a problem? Is it okay for aliasing to happen in tree? but that will cause typetest breaks.
There was a problem hiding this comment.
Wrong link about the aliasing bit? Not sure what the concern is there.
There was a problem hiding this comment.
Aliasing in core-interfaces: 04274d8
EDIT:
Aliasing is not required. Types can be exported from core-interfaces by adding eslint suppression. 4d0a841
When importing types from tree, VSCode displays the usual strikethrough. However, the TSDoc shown is from core-interfaces rather than the deprecation warning from tree. I guess that's OK?
@Josmithr, is there anyway to add the deprecation docs too?
export type {
/**
* Moved to the `@fluidframework/core-interfaces` package.
* @deprecated Moved to the `@fluidframework/core-interfaces` package.
*/
Listeners,
/**
* @deprecated Moved to the `@fluidframework/core-interfaces` package.
*/
IsListener,
/**
* @deprecated Moved to the `@fluidframework/core-interfaces` package.
*/
Listenable,
/**
* Moved to the `@fluidframework/core-interfaces` package.
* @deprecated Moved to the `@fluidframework/core-interfaces` package.
*/
Off,
} from "@fluidframework/core-interfaces";Similar example: https://github.com/microsoft/FluidFramework/blame/main/packages/common/container-definitions/src/index.ts#L70
There was a problem hiding this comment.
I'm actually not sure why Intellisense picks up on the deprecation notice and nothing else. For what it's worth, our API docs will not understand this. Unfortunately, I think you would need to do what @alexvy86 suggested to make everything work out correctly. E.g.
import { Foo as FooBase } from "a";
/**
* Foo description.
* @deprecated ...
*/
export type Foo = FooBase;I don't believe you can use {@inheritDoc} in this case, unfortunately, since we're introducing local documentation (the deprecated tag).
Are all of these re-exports type-only? Or are any of them classes or anything? Making the re-export of a class would require subclassing to remain compatible.
There was a problem hiding this comment.
They are all type-only.
| type Listenable, | ||
| type Off, | ||
| } from "./events/index.js"; | ||
| export type { |
There was a problem hiding this comment.
Removing these will be a breaking change on public API surface so it'll have to wait until 3.0, I think (?) but we'll have to deprecate them somehow before that, no? Should we do something like importing them as Listeners_base or similar in this package, and exporting them from here as a new type with its own docs and deprecation notice? I.e. something like (double check it, not sure it works as written):
import { Listeners as Listeners_base } from "@fluidframework/core-interfaces";
/**
* @inheritDoc <something here, I don't recall the syntax>
* @ deprecated This type has moved to `@fluidframework/core-interfaces` and will be removed from here in a future release.
* If you need to reference it directly, do it from `@fluidframework/core-interfaces`.
*/
export type Listeners = Listeners_base;
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
This change relocates event library from SharedTree to client-utils and core-interfaces.
Old PR: #23037
ADO#6595