8000 fix(cdk-render-strategies): don't send complete event to strategy-behavior by hoebbelsB · Pull Request #954 · rx-angular/rx-angular · GitHub
[go: up one dir, main page]
Skip to content

fix(cdk-render-strategies): don't send complete event to strategy-behavior #954

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 23, 2021
Merged

fix(cdk-render-strategies): don't send complete event to strategy-behavior #954

merged 2 commits into from
Sep 23, 2021

Conversation

hoebbelsB
Copy link
Member

Description

The current implementation of onStrategy returns of(value) and attaches the strategies behavior to it. Since of completes immediately, all strategies that use coalesceWith will just emit synchronously a value. This currently affects the local strategy which is why it was always re-defined by immediate.

  • send complete event only after the strategies behavior emitted a value by using take(1)
  • implement tests for onStrategy

before:
The work is performed synchronously:

image

after:
You can see that the work was actually scheduled to the next animationFrame:

image

@hoebbelsB hoebbelsB requested a review from BioPhoton September 20, 2021 16:16
@github-actions github-actions bot added 🛂 Test Unit tests, e2e tests, integration tests, test coverage 🛠️ CDK CDK related labels Sep 20, 2021
@nx-cloud
Copy link
nx-cloud bot commented Sep 20, 2021 •
edited
Loading

Nx Cloud Report

CI ran the following commands for commit 3951021. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx affected:build --with-deps
#000000 nx run ssr-e2e:e2e --headless
#000000 nx affected:test --parallel --maxParallel=2
#000000 nx affected:lint --parallel --maxParallel=3

Sent with 💌 from NxCloud.

BioPhoton
BioPhoton approved these changes Sep 23, 2021
View reviewed changes
Copy link
Member
@BioPhoton BioPhoton left a comment

Choose a reason for hiding this comment

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

nice we have this solved now. Also thanks for the tests!

@BioPhoton BioPhoton merged commit 8df99ea into master Sep 23, 2021
@BioPhoton BioPhoton deleted the fix-local-strategy branch September 23, 2021 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BioPhoton BioPhoton BioPhoton approved these changes

@edbzn edbzn Awaiting requested review from edbzn

@Karnaukhov-kh Karnaukhov-kh Awaiting requested review from Karnaukhov-kh

Assignees
No one assigned
Labels
🛠️ CDK CDK related 🛂 Test Unit tests, e2e tests, integration tests, test coverage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hoebbelsB @BioPhoton
0