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

build(client): generate legacy API report #21634

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

jason-ha
Copy link
Contributor

client-utils is in the @fluid-internal scope and thus does not have trimmed exports. But it currently support legacy APIs for typed event emitter.

Setup a non-production legacy "roll up" file for both the browser and Node.js entrypoints. Generate API reports and perform linting like @fluidframework scoped packages with /legacy, but using the faux "roll ups".

Additionally now verify API reports for browser and Node.js entrypoints are identical.

Note: api task and api-extractor named scripts are not used as in @fluidframework scoped packages as these scripts are not part of production API. They are only used for docs and linting which have task dependencies defined.

client-utils is in the `@fluid-internal` scope and thus does not have trimmed exports. But it currently support legacy APIs for typed event emitter.

Setup a non-production legacy "roll up" file for both the browser and Node.js entrypoints. Generate API reports and perform linting like `@fluidframework` scoped packages with /legacy, but using the faux "roll ups".

Additionally now verify API reports for browser and Node.js entrypoints are identical.

Note: `api` task and `api-extractor` named scripts are not used as in `@fluidframework` scoped packages as these scripts are not part of production API. They are only used for docs and linting which have task dependencies defined.
@jason-ha jason-ha requested a review from a team as a code owner June 26, 2024 15:35
@github-actions github-actions bot added public api change Changes to a public API base: main PRs targeted against main branch labels Jun 26, 2024
Also use same path tree will need to use (to be node10 compat capable - tooling defect)
@@ -9,8 +9,6 @@ export { EventEmitter }
// @alpha
export type EventEmitterEventType = string;

export { performance_2 as performance }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an @internal export, right? So this change is effectively a correction of this API-Extractor bug?

@Josmithr
Copy link
Contributor

client-utils is in the @fluid-internal scope and thus does not have trimmed exports. But it currently support legacy APIs for typed event emitter.

What is our plan for TypedEventEmitter? Do we need to export it for Loop? Could we just copy it into their codebase and be done with it?

Copy link
Contributor

@Josmithr Josmithr left a comment

Choose a reason for hiding this comment

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

Changes look good to me, assuming we need to support TypedEventEmitter for Loop. But I do wonder if it would be reasonable for us to just copy TypedEventEmitter into their code and let them maintain their own copy of it.

Presumably we also export (alpha+) classes that extend this? So presumably there is more blocking us from making the type @internal, but at least then we could remove any remaining direct dependencies on our @fluid-internal package from Loop?

@msfluid-bot
Copy link
Collaborator

Baseline CI build failed, cannot generate bundle analysis at this time


Baseline commit: d976469

Generated by 🚫 dangerJS against fb49f66

@jason-ha
Copy link
Contributor Author

client-utils is in the @fluid-internal scope and thus does not have trimmed exports. But it currently support legacy APIs for typed event emitter.

What is our plan for TypedEventEmitter? Do we need to export it for Loop? Could we just copy it into their codebase and be done with it?

There is some work planned around event emitter cleanup which I think will relocate it from this package. That is tracked as either or both AB#7377 and AB#6595. But I don't think it will go away anytime soon because of the way classes are exported for telemetry-utils and some test mock classes. Gotta have factory functions to hide classes that extend TypedEventEmitter.

@jason-ha jason-ha merged commit 3931af2 into microsoft:main Jun 26, 2024
30 checks passed
@jason-ha jason-ha deleted the client-utils-legacy-api-report branch June 26, 2024 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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