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

AdHocFiltersVariable: provide updateFilters method to allow updating filters without emitting SceneVariableValueChangedEvent #1004

Merged
merged 10 commits into from
Dec 20, 2024
53 changes: 53 additions & 0 deletions packages/scenes/src/variables/adhoc/AdHocFiltersVariable.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -958,6 +958,59 @@ describe.each(['11.1.2', '11.1.1'])('AdHocFiltersVariable', (v) => {
expect(evtHandler).not.toHaveBeenCalled();
});

it('updateFilters should not publish event when expr did not change', () => {
const variable = new AdHocFiltersVariable({
datasource: { uid: 'hello' },
applyMode: 'manual',
filters: [{ key: 'key1', operator: '=', value: 'val1' }],
});

variable.activate();

const evtHandler = jest.fn();
variable.subscribeToEvent(SceneVariableValueChangedEvent, evtHandler);

variable.updateFilters({ filters: variable.state.filters.slice(0) });

expect(evtHandler).not.toHaveBeenCalled();
});

it('updateFilters should publish event on when expr did change', () => {
const variable = new AdHocFiltersVariable({
datasource: { uid: 'hello' },
applyMode: 'manual',
filters: [{ key: 'key1', operator: '=', value: 'val1' }],
});

variable.activate();

const evtHandler = jest.fn();
variable.subscribeToEvent(SceneVariableValueChangedEvent, evtHandler);

variable.updateFilters({ filters: [{ key: 'key2', operator: '=', value: 'val1' }] });

expect(evtHandler).toHaveBeenCalled();
expect(variable.state.filterExpression).toEqual(`key2="val1"`);
});

it('updateFilters should not publish event on when expr did change, if skipPublish is true', () => {
const variable = new AdHocFiltersVariable({
datasource: { uid: 'hello' },
applyMode: 'manual',
filters: [{ key: 'key1', operator: '=', value: 'val1' }],
});

variable.activate();

const evtHandler = jest.fn();
variable.subscribeToEvent(SceneVariableValueChangedEvent, evtHandler);

variable.updateFilters({ filters: [{ key: 'key2', operator: '=', value: 'val1' }] }, { skipPublish: true });

expect(evtHandler).not.toHaveBeenCalled();
expect(variable.state.filterExpression).toEqual(`key2="val1"`);
});

it('Should create variable with applyMode as manual by default and it allows to override it', () => {
const defaultVariable = new AdHocFiltersVariable({
datasource: { uid: 'hello' },
Expand Down
18 changes: 16 additions & 2 deletions packages/scenes/src/variables/adhoc/AdHocFiltersVariable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -190,16 +190,30 @@ export class AdHocFiltersVariable
}

public setState(update: Partial<AdHocFiltersVariableState>): void {
this.updateFilters(update);
}

/**
* Updates the variable's `filters` and `filterExpression` state.
* If `skipPublish` option is true, this will not emit the `SceneVariableValueChangedEvent`,
* allowing consumers to update the filters without triggering dependent data providers.
*/
public updateFilters(
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should adjust updateFilters to:

 public updateFilters(
    update: Partial<AdHocFiltersVariableState>,
    options?: {
      skipPublish?: boolean;
    }
  )....

and then reuse that in the setState so we don't get out of sync on functions and have a single source of truth for updating

  public setState(update: Partial<AdHocFiltersVariableState>): void {
    this.updateFilters(update);
  }

Copy link
Contributor Author

@gtk-grafana gtk-grafana Dec 19, 2024

Choose a reason for hiding this comment

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

Good catch, this change is implemented in cdffe34

Copy link
Member

@dprokop dprokop Dec 20, 2024

Choose a reason for hiding this comment

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

I disagree with this. updateFilters should update filters, not entire state of the object. What i think is needed here is to remove the setState overload all together. If someone want's to update filters (and control behavior of such updatr), then we have an API for that. If someone wants to update the state, then they can rely on the base class's setState (without side effect of the event being published). This is a breaking change, hence i suggested adding release notes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @dprokop, the setState overload has been reverted, but I kept the option to force the publish in the updateFilters.

update: Partial<AdHocFiltersVariableState>,
options?: {
skipPublish?: boolean;
}
): void {
let filterExpressionChanged = false;

if (update.filters && update.filters !== this.state.filters && !update.filterExpression) {
if (update.filters && update.filters !== this.state.filters) {
gtk-grafana marked this conversation as resolved.
Show resolved Hide resolved
update.filterExpression = renderExpression(this.state.expressionBuilder, update.filters);
filterExpressionChanged = update.filterExpression !== this.state.filterExpression;
}

super.setState(update);

if (filterExpressionChanged) {
if (filterExpressionChanged && options?.skipPublish !== true) {
this.publishEvent(new SceneVariableValueChangedEvent(this), true);
}
}
Expand Down
Loading