-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Control group state diffing #189128
Control group state diffing #189128
Conversation
nreese
commented
Jul 24, 2024
•
edited
Loading
edited
/ci |
@elasticmachine merge upstream |
/ci |
/ci |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
...resentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts
Outdated
Show resolved
Hide resolved
...resentation/presentation_containers/interfaces/unsaved_changes/initialize_unsaved_changes.ts
Show resolved
Hide resolved
...controls_example/public/react_controls/timeslider_control/get_timeslider_control_factory.tsx
Outdated
Show resolved
Hide resolved
...ls_example/public/react_controls/data_controls/search_control/get_search_control_factory.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one nit about range slider, but nothing blocking - woohoo! Once we add #189580, I think controls will be in a really good state 🤞
...xample/public/react_controls/data_controls/range_slider/get_range_slider_control_factory.tsx
Outdated
Show resolved
Hide resolved
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @nreese |
Closes #190066 ## Summary In #189128 (comment), I suggested adding a `skip` to the comparators subscription in order to prevent `unsavedChanges` from emitting twice - however, while I was on the right track with this suggestion, I didn't look closely enough at how this worked. Essentially, this is how it worked without the skip when creating a brand new React embeddable: 1. We initialize `unsavedChanges` by calling `runComparators` to get its initial value - since it is a new embeddable and therefore **does not have** unsaved state, it returns the entire state per the early return in `runComparators` 2. The comparator subjects all emit and, after the debounce, `unsavedChanges` gets set **again** by calling `runComparators` - similarly, it returns the entire state due to the early return. 3. Unsaved changes emits twice unnecessarily - once at initialization, once after a debounce 🔥 By adding the `skip` where I suggested, this is what happened: 1. We initialize `unsavedChanges` by calling `runComparators` to get its initial value - it returns the entire state due to the early return. 2. We **ignore** the first emit of every comparator due to the `skip` - this is good, because unsaved changes emits only once! 3. But, when we go to save the dashboard, the placement of the `skip` made it so that the subscription **did not fire** when `lastSavedState` emits - it is waiting for the comparators to emit again before firing! So, the unsaved changes remains unnecessarily 🔥 Essentially, we want to `skip` the first emit of the **whole** `comparator` + `lastSavedState` observable that results from the `pipe` - **not** just the comparators. Otherwise, we don't get to the `combineLatestWith` until **after** the comparators emit a second time, which means we don't respond when`lastSavedState` emits after a React embeddable panel is freshly added. By rearranging the order of operations in the `pipe`, we achieve the desired behaviour. To test the above, move the `skip` to **before** the `lastSavedState` emit. Add a React embeddable panel, and try to save - unsaved changes will be stuck. On a fresh dashboard, add a React embeddable panel again, **edit** that React embeddable to trigger a comparator emit, **then** save - unsaved changes is cleared! ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)