Deprecate types in tree #23313

sonalideshpandemsft · 2024-12-12T06:33:08Z

Add a changeset to deprecate types from @fluidframework/tree. Modify fluid-framework to re-export the non-deprecated APIs from core-interfaces.

Added basic test cases which covers importing a type from the three packages: tree, core-interfaces, and fluid-framework. These tests are added under examples.

Prior PR: #23183

ADO#25440

jason-ha · 2024-12-12T10:02:58Z

packages/framework/fluid-framework/src/index.ts

+	// The event APIs are known to conflict, and this is intended as the exports via `@fluidframework/core-interfaces` are preferred over the deprecated ones from `@fluidframework/tree`.
+	// eslint-disable-next-line import/export
+	Listeners,
+	// The event APIs are known to conflict, and this is intended as the exports via `@fluidframework/core-interfaces` are preferred over the deprecated ones from `@fluidframework/tree`.
+	// eslint-disable-next-line import/export
+	IsListener,
+	// The event APIs are known to conflict, and this is intended as the exports via `@fluidframework/core-interfaces` are preferred over the deprecated ones from `@fluidframework/tree`.
+	// eslint-disable-next-line import/export
+	Listenable,
+	//  The event APIs are known to conflict, and this is intended as the exports via `@fluidframework/core-interfaces` are preferred over the deprecated ones from `@fluidframework/tree`.
+	// eslint-disable-next-line import/export
+	Off,
 } from "@fluidframework/core-interfaces";

 export type { isFluidHandle } from "@fluidframework/runtime-utils";

 // Let the tree package manage its own API surface.
 // Note: this only surfaces the `@public, @beta and @alpha` API items from the tree package.
-// eslint-disable-next-line no-restricted-syntax, import/no-internal-modules
+// This reexports all non-conflicting APIs from `@fluidframework/tree`. The event APIs are known to conflict, and this is intended as the exports via `@fluidframework/core-interfaces` are preferred over the deprecated ones from `@fluidframework/tree`.
+// eslint-disable-next-line no-restricted-syntax, import/no-internal-modules, import/export


I don't think these comments are sufficient. There will need to be statements about which prevail and why as it is uncommon and plenty of readers won't know the answer. I still don't know the answer.

You could add something like: "When * exports conflict with named exports, the named export is used (and this lint warning is triggered). This allows the non deprecated versions of the events (above as named exports) to replace the deprecated ones from tree."

@CraigMacomber, is there a spec / docs we can reference?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/export says "If there are two wildcard exports statements that implicitly re-export the same name, neither one is re-exported." But I don't see the text about named exports win.

I found that same document, and was also disappointed due to the same omission so I did some experiments.

I kinda assumed this change would have tests to make sure the exports actually work (compile and work at runtime), I guess it doesn't so those should probably be added.

Note that should the approach taken here not work, blocking the undesired exports with a second wildcard export, then adding them a third time as named exports should be fine according to that document as far as I can tell.

I tried to understand the actual spec https://tc39.es/ecma262/multipage/ecmascript-language-scripts-and-modules.html#sec-exports but I couldn't figure it out.

I think perhaps in https://tc39.es/ecma262/multipage/ecmascript-language-scripts-and-modules.html#sec-getexportednames step 8.d.i.1 implies it works as I claimed?

I had also tried reading the same "sec-exports" page and could not derive the intended/prescribed behavior.
I don't see how second * wildcard would work well/easily. I think it means crafting an extra file in fluid-framework that has just the conflicts, then index exports * from that to cause specific conflicts, then also a named set.

I would favor the other version where there are no conflicts if the spec is complicated or hard to even find. I would not be surprised if some implementation got it wrong. I don't think there will be a lot of maintenance once the structure is in place (with appropriate comments for dev guidance). An alternative would be to explicitly name tree exports like we do for every other package. The other PR gives us compromise version of that option in tree itself - so engineers don't have to hop packages.

(hmm github is not showing me all of the comments in timely fashion - reading other spec soon)

I think yes it works as you claim, Craig, because all of 7 (IndirectExportEntries) happens before 8. What I don't see in there is support of the MDN note that two wildcards in conflict resolve to none. Per the spec, I would expect the first encountered to win. (Makes me a little concerned about just relying on spec, but not too much.) I am good with either solution with some preference for the explicit handling ourselves.
I think as tree reviewer, Craig, you will get the biggest vote.
Should we choose this and then determine there is a problem, we have an option to fix with the other pattern.

.changeset/ready-seas-ask.md

packages/framework/fluid-framework/src/index.ts

Copilot reviewed 3 out of 3 changed files in this pull request and generated no suggestions.

Comments skipped due to low confidence (5)

packages/framework/fluid-framework/src/index.ts:57

[nitpick] The comment explaining the ESLint disable directive is quite long and could be split into multiple lines for better readability.

/* eslint-disable import/export -- The event APIs are known to conflict, and this is intended as the exports via `@fluidframework/core-interfaces` are preferred over the deprecated ones from `@fluidframework/tree`. */

packages/dds/tree/src/index.ts:21

The type name 'Listeners' is too generic and can cause confusion. Consider renaming it to something more specific like 'EventListeners'.

Listeners,

packages/dds/tree/src/index.ts:25

The type name 'IsListener' is ambiguous. Consider renaming it to something more descriptive like 'IsEventListener'.

IsListener,

packages/dds/tree/src/index.ts:29

The type name 'Listenable' is too generic and can cause confusion. Consider renaming it to something more specific like 'EventListenable'.

Listenable,

packages/dds/tree/src/index.ts:33

The type name 'Off' is too generic and can cause confusion. Consider renaming it to something more specific like 'EventOff'.

Off,

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

packages/framework/fluid-framework/src/index.ts

.changeset/ready-seas-ask.md

jason-ha · 2024-12-16T19:04:24Z

packages/dds/tree/src/index.ts

 export type {
+	/**
+	 * @deprecated Deprecated in `@fluidframework/tree`. Consider importing from `fluid-framework` or `@fluidframework/core-interfaces` instead.
+	 */
 	Listeners,
+	/**
+	 * @deprecated Deprecated in `@fluidframework/tree`. Consider importing from `fluid-framework` or `@fluidframework/core-interfaces` instead.
+	 */
 	IsListener,
+	/**
+	 * @deprecated Deprecated in `@fluidframework/tree`. Consider importing from `fluid-framework` or `@fluidframework/core-interfaces` instead.
+	 */
 	Listenable,
+	/**
+	 * @deprecated Deprecated in `@fluidframework/tree`. Consider importing from `fluid-framework` or `@fluidframework/core-interfaces` instead.
+	 */
 	Off,
 } from "@fluidframework/core-interfaces";


Looks like this is actually insufficient. While tsc supports this pattern, api-extactor and eslint do not. Notice that there is no API report change here.

The same applies to the previous approach—there are no API Extractor updates for those changes either. But it shows the usual strikethrough in VSCode to indicate these types are deprecated.

One approach is to copy these types in tree, mark them as deprecated, and export them from there. This will update the api-extractor reports to reflect the deprecation status of these types.

github-actions · 2024-12-17T11:08:56Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  170006 links
    1595 destination URLs
    1825 URLs ignored
       0 warnings
       0 errors

feedback

cbb3ba5

sonalideshpandemsft requested review from a team as code owners December 12, 2024 06:33

github-actions bot added base: main PRs targeted against main branch area: dds Issues related to distributed data structures area: dds: tree area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct changeset-present labels Dec 12, 2024

sonalideshpandemsft mentioned this pull request Dec 12, 2024

Deprecate types in tree and re-export from fluid-framework #23183

Open

sonalideshpandemsft requested review from CraigMacomber and jason-ha December 12, 2024 06:36

jason-ha reviewed Dec 12, 2024

View reviewed changes

CraigMacomber reviewed Dec 12, 2024

View reviewed changes

.changeset/ready-seas-ask.md Outdated Show resolved Hide resolved

CraigMacomber reviewed Dec 12, 2024

View reviewed changes

.changeset/ready-seas-ask.md Outdated Show resolved Hide resolved

CraigMacomber reviewed Dec 12, 2024

View reviewed changes

packages/framework/fluid-framework/src/index.ts Outdated Show resolved Hide resolved

feedback

ba4ebed

frankmueller-msft requested a review from Copilot December 12, 2024 23:08

Copilot AI reviewed Dec 12, 2024

View reviewed changes

jason-ha reviewed Dec 13, 2024

View reviewed changes

packages/framework/fluid-framework/src/index.ts Outdated Show resolved Hide resolved

jason-ha reviewed Dec 13, 2024

View reviewed changes

.changeset/ready-seas-ask.md Outdated Show resolved Hide resolved

jason-ha reviewed Dec 13, 2024

View reviewed changes

.changeset/ready-seas-ask.md Outdated Show resolved Hide resolved

sonalideshpandemsft added 2 commits December 16, 2024 12:21

feedback

530a6fa

Merge remote-tracking branch 'origin' into deprecate-events-types

27ec0d6

github-actions bot added area: examples Changes that focus on our examples dependencies Pull requests that update a dependency file labels Dec 16, 2024

Add tests

c6455be

sonalideshpandemsft force-pushed the deprecate-events-types branch from bc86952 to c6455be Compare December 16, 2024 17:46

jason-ha requested changes Dec 16, 2024

View reviewed changes

sonalideshpandemsft requested a review from a team as a code owner December 17, 2024 10:52

github-actions bot added the public api change Changes to a public API label Dec 17, 2024

sonalideshpandemsft force-pushed the deprecate-events-types branch from 59a4ebe to 0594542 Compare December 17, 2024 11:00

sonalideshpandemsft force-pushed the deprecate-events-types branch from 0594542 to c6455be Compare December 17, 2024 17:37

github-actions bot removed the public api change Changes to a public API label Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate types in tree #23313

Deprecate types in tree #23313

sonalideshpandemsft commented Dec 12, 2024 •

edited

Loading

jason-ha Dec 12, 2024

CraigMacomber Dec 12, 2024

jason-ha Dec 12, 2024

CraigMacomber Dec 13, 2024

CraigMacomber Dec 13, 2024

CraigMacomber Dec 13, 2024

jason-ha Dec 13, 2024

jason-ha Dec 13, 2024

jason-ha Dec 13, 2024

jason-ha Dec 16, 2024

sonalideshpandemsft Dec 17, 2024

sonalideshpandemsft Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024

Deprecate types in tree #23313

Are you sure you want to change the base?

Deprecate types in tree #23313

Conversation

sonalideshpandemsft commented Dec 12, 2024 • edited Loading

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

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 Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2024

linkcheck output

sonalideshpandemsft commented Dec 12, 2024 •

edited

Loading

sonalideshpandemsft Dec 17, 2024 •

edited

Loading