Skip to content

Add off() method to Listenable event interface#23046

Merged
noencke merged 7 commits into
microsoft:mainfrom
noencke:event-off
Nov 12, 2024
Merged

Add off() method to Listenable event interface#23046
noencke merged 7 commits into
microsoft:mainfrom
noencke:event-off

Conversation

@noencke
Copy link
Copy Markdown
Contributor

@noencke noencke commented Nov 9, 2024

Description

This exposes the off() method on the Listenable event interface, providing an additional way to unsubscribe from events. The current way of unsubscribing, via the deregistration function returned by on(), is still supported. Having both options available is meant to give customers options without being opinionated about the best pattern, as it depends on preference and use case.

To prevent the two strategies from interfering with each other, it has also been made illegal to register the same listener more than once for the same event. It is now documented as throwing an error rather than as undefined behavior.

This also adds a test for registering events with keys that are JS symbols and does some minor doc cleanup.

@noencke noencke requested review from a team as code owners November 9, 2024 01:13
@github-actions github-actions Bot added 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 public api change Changes to a public API base: main PRs targeted against main branch labels Nov 9, 2024
Copy link
Copy Markdown
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

↑ packages.dds.tree.src.events:
Line Coverage Change: -1.68%    Branch Coverage Change: 1.88%
Metric NameBaseline coveragePR coverageCoverage Diff
Branch Coverage 91.66% 93.54% ↑ 1.88%
Line Coverage 71.65% 69.97% ↓ -1.68%

Baseline commit: e0e9c89
Baseline build: 305911
Happy Coding!!

Code coverage comparison check failed!!
More Details: Readme

  • Skip This Check!!

What to do if the code coverage check fails:

  • Ideally, add more tests to increase the code coverage for the package(s) whose code-coverage regressed.

  • If a regression is causing the build to fail and is due to removal of tests, removal of code with lots of tests or any other valid reason, there is a checkbox further up in this comment that determines if the code coverage check should fail the build or not. You can check the box and trigger the build again. The test coverage analysis will still be done, but it will not fail the build if a regression is detected.

  • Unchecking the checkbox and triggering another build should go back to failing the build if a test-coverage regression is detected.

  • You can check which lines are covered or not covered by your tests with these steps:

    • Go to the PR ADO build.
    • Click on the link to see its published artifacts. You will see an artifact named codeCoverageAnalysis, which you can expand to reach to a particular source file's coverage html which will show which lines are covered/not covered by your tests.
    • You can also run different kind of tests locally with :coverage tests commands to find out the coverage.

Comment thread packages/dds/tree/src/test/events/events.spec.ts Outdated
Comment thread packages/dds/tree/src/test/events/events.spec.ts Outdated
Comment thread packages/dds/tree/src/test/events/events.spec.ts Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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

Files not reviewed (9)
  • packages/dds/tree/api-report/tree.legacy.public.api.md: Evaluated as low risk
  • packages/framework/fluid-framework/api-report/fluid-framework.public.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.legacy.alpha.api.md: Evaluated as low risk
  • packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md: Evaluated as low risk
  • packages/dds/tree/api-report/tree.public.api.md: Evaluated as low risk
  • packages/dds/tree/api-report/tree.alpha.api.md: Evaluated as low risk
  • packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md: Evaluated as low risk
  • packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.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

Comment thread packages/dds/tree/src/events/emitter.ts Outdated
Comment thread packages/dds/tree/src/test/events/events.spec.ts
Comment thread packages/dds/tree/src/test/events/events.spec.ts
Copy link
Copy Markdown
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

Thanks for making even better test code (better examples for all)

@github-actions
Copy link
Copy Markdown
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluidframework-docs@0.25.0 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"


> fluidframework-docs@0.25.0 ci:start
> http-server ./public --port 1313 --silent


> fluidframework-docs@0.25.0 linkcheck:full
> npm run linkcheck:fast -- --external


> fluidframework-docs@0.25.0 linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  443362 links
    3411 destination URLs
       2 URLs ignored
       0 warnings
       0 errors


@msfluid-bot
Copy link
Copy Markdown
Collaborator

@fluid-example/bundle-size-tests: +633 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.33 KB 464.36 KB +35 Bytes
azureClient.js 563.18 KB 563.23 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 262.48 KB 262.5 KB +14 Bytes
fluidFramework.js 426.65 KB 426.85 KB +208 Bytes
loader.js 134.18 KB 134.19 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.36 KB 148.36 KB +7 Bytes
odspClient.js 528.97 KB 529.02 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 164.26 KB 164.27 KB +7 Bytes
sharedTree.js 417.11 KB 417.3 KB +201 Bytes
Total Size 3.37 MB 3.37 MB +633 Bytes

Baseline commit: e0e9c89

Generated by 🚫 dangerJS against 6cb4f5e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: dds: tree area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch changeset-present public api change Changes to a public API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants