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

refactor(core): introduce eventsubject to replace internal observable #2590

Merged
merged 24 commits into from
Jun 26, 2024

Conversation

wzhudev
Copy link
Member

@wzhudev wzhudev commented Jun 22, 2024

In this PR, we (@lumixraku & me) removed the legacy reactive programming pattern Observable and Observer from the core package to use custom EventSubject instead. Not only it keeps the stop propagation functionality the old Observable provided, but also leverages RxJS.

Pull Request Checklist

  • Related tickets or issues have been linked in the PR description (or missing issue).
  • Naming convention is followed (do please check it especially when you created new plugins, commands and resources).
  • Unit tests have been added for the changes (if applicable).
  • Breaking changes have been documented (or no breaking changes introduced in this PR).

@wzhudev wzhudev changed the title refactor(core): remove observer redundant code refactor(core): remove legacy observer and use rxjs instead Jun 22, 2024
Copy link

codecov bot commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 2.57985% with 793 lines in your changes missing coverage. Please review.

Project coverage is 26.95%. Comparing base (8beb8b0) to head (548a49a).
Report is 2 commits behind head on dev.

Files Patch % Lines
...ender-controllers/header-move.render-controller.ts 0.00% 116 Missing ⚠️
...der-controllers/header-resize.render-controller.ts 0.00% 72 Missing ⚠️
packages/engine-render/src/scene.transformer.ts 0.00% 71 Missing ⚠️
packages/engine-render/src/base-object.ts 0.00% 55 Missing ⚠️
packages/engine-render/src/engine.ts 0.00% 36 Missing ⚠️
packages/engine-render/src/thin-scene.ts 0.00% 36 Missing ⚠️
...ender-controllers/header-menu.render-controller.ts 0.00% 36 Missing ⚠️
packages/core/src/observer/observable.ts 2.77% 35 Missing ⚠️
...ers/render-controllers/freeze.render-controller.ts 0.00% 34 Missing ⚠️
packages/engine-render/src/scene.ts 0.00% 31 Missing ⚠️
... and 53 more
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #2590      +/-   ##
==========================================
+ Coverage   26.93%   26.95%   +0.01%     
==========================================
  Files        1690     1692       +2     
  Lines       85230    85155      -75     
  Branches    17727    17736       +9     
==========================================
- Hits        22955    22951       -4     
+ Misses      62275    62204      -71     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jun 22, 2024

View Deployment

📑 Examples 📚 Storybook
🔗 Preview link 🔗 Preview link

@wzhudev wzhudev marked this pull request as ready for review June 22, 2024 05:48
@wzhudev wzhudev changed the title refactor(core): remove legacy observer and use rxjs instead refactor(core): introduce eventsubject to replace internal observable Jun 22, 2024
@lumixraku lumixraku force-pushed the refactor/event-observer branch 8 times, most recently from 899fb27 to 22c2106 Compare June 25, 2024 04:10
@lumixraku lumixraku added the qa:untested This PR is ready to be tested label Jun 25, 2024
@wzhudev wzhudev merged commit d196458 into dev Jun 26, 2024
9 of 10 checks passed
@wzhudev wzhudev deleted the refactor/event-observer branch June 26, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qa:untested This PR is ready to be tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants