Chore: Remove angular dependency from data sources #27735

kaydelaney · 2020-09-23T16:03:25Z

What this PR does / why we need it:
Removes the use of @ngInject for dependency injection from data sources. Dependency injection is still used, but is done through default parameters. In some cases this can simplify testing since the ceremony of jest mocks is no longer required.
Builds upon some earlier work by @hugohaggmark (#23048)

hugohaggmark · 2020-09-24T04:20:23Z

public/app/features/dashboard/services/TimeSrv.ts

@@ -316,3 +316,5 @@ export function getTimeSrv(): TimeSrv {
 }

 coreModule.service('timeSrv', TimeSrv);
+
+export default getTimeSrv();


I think we should try to avoid default exports.

Aha ok, I see what you're doing in the rest of the PR.

hugohaggmark

Looks great but I wasn't aware that we had exported TemplateSrv as default which is a pattern that we've previously tried to avoid.

Another thing that came up during the review is that datasource plugins should depend on @grafana/runtime and not files under app/features...

torkelo

Nice!

yes, if possible would be better to get function and types from @grafana/runtime (but might be some cases where there are functions that are used that are not part of the public interface).

kaydelaney · 2020-09-24T12:45:11Z

There are indeed some situations where data sources use TemplateSrv methods that aren't part of the public interface, for example the influx data source makes use of templateSrv.variableExists and templateSrv.getAdhocFilters.
Should these methods perhaps get added to the public interface? @torkelo

torkelo · 2020-09-24T12:48:59Z

For those cases you can keep using the internal ones for now.

torkelo · 2020-09-24T12:49:45Z

But I would still avoid export default

torkelo

Looks good. Not super happy with getTemplateSrv().replace in many places harder to read than templateSrv.replace . The idea behind the get functions was to help in jest mocks and with lazy eval of a singleston but this was before I knew you can have module variables that you can change and have that impact all places that already imported that variable.

So we could technically stop with the get functions and just export/import the singleton variable instead. Maybe a good topic to discuss on a frontend weekly :)

hugohaggmark · 2020-09-25T07:01:09Z

Shouldn't we try to use grafana/runtime in all the cases except the ones that use more then the exported functions?

dprokop · 2020-09-25T07:22:27Z

Shouldn't we try to use grafana/runtime in all the cases except the ones that use more then the exported functions?

I think we should and I believe this is what we did until now.

hugohaggmark · 2020-09-25T11:34:37Z

public/app/features/dashboard/components/DashboardRow/DashboardRow.tsx

@@ -69,7 +69,7 @@ export class DashboardRow extends React.Component<DashboardRowProps, any> {
      'dashboard-row--collapsed': this.state.collapsed,
    });

-    const title = templateSrv.replace(this.props.panel.title, this.props.panel.scopedVars, 'text');
+    const title = getTemplateSrv().replace(this.props.panel.title, this.props.panel.scopedVars, 'text');


This component uses only the replace function, should be ok to just import getTemplateSrv from grafana/runtime?

hugohaggmark · 2020-09-25T11:35:37Z

public/app/features/dashboard/dashgrid/PanelHeader/PanelHeader.tsx

@@ -131,7 +131,7 @@ export class PanelHeader extends Component<Props, State> {
  render() {
    const { panel, scopedVars, error, isViewing, isEditing, data, alertState } = this.props;
    const { menuItems } = this.state;
-    const title = templateSrv.replace(panel.title, scopedVars, 'text');
+    const title = getTemplateSrv().replace(panel.title, scopedVars, 'text');


This component uses only the replace function, should be ok to just import getTemplateSrv from grafana/runtime?

hugohaggmark · 2020-09-25T11:37:43Z

@kaydelaney maybe I'm just tired :) but I can still find components that only use the replace function which means that we could use the getTemplateSrv from grafana/runtime

hugohaggmark · 2020-09-28T07:59:25Z

public/app/features/variables/query/actions.ts

@@ -138,5 +138,5 @@ const getTemplatedRegex = (variable: QueryVariableModel): string => {
    return '';
  }

-  return templateSrv.replace(variable.regex, {}, 'regex');
+  return getTemplateSrv().replace(variable.regex, {}, 'regex');


Maybe runtime import for this one?

public/app/plugins/datasource/cloudwatch/query_parameter_ctrl.ts

hugohaggmark · 2020-09-28T08:06:12Z

public/app/plugins/datasource/loki/result_transformer.ts

@@ -245,7 +245,7 @@ export function createMetricLabel(labelData: { [key: string]: string }, options?
  let label =
    options === undefined || _.isEmpty(options.legendFormat)
      ? getOriginalMetricName(labelData)
-      : renderTemplate(templateSrv.replace(options.legendFormat ?? '', options.scopedVars), labelData);
+      : renderTemplate(getTemplateSrv().replace(options.legendFormat ?? '', options.scopedVars), labelData);


This seems to be a runtime import too

hugohaggmark

Fantastic work Kay, found some more places that I think could use a runtime import

Also uses @grafana/runtime versions of the interfaces where possible

* Chore: Remove angular dependency from data sources * Removes default export for time and template srvs Also uses @grafana/runtime versions of the interfaces where possible * Replace usage of internal templateSrv where possible * Use runtime templateSrv in a couple more places

kaydelaney added the type/chore label Sep 23, 2020

kaydelaney requested review from hugohaggmark and a team September 23, 2020 16:03

kaydelaney requested review from a team as code owners September 23, 2020 16:03

kaydelaney self-assigned this Sep 23, 2020

kaydelaney added this to In Review in Frontend Platform Backlog via automation Sep 23, 2020

kaydelaney requested review from peterholmberg, aknuds1, kylebrandt and aocenas and removed request for a team September 23, 2020 16:03

aknuds1 removed their request for review September 23, 2020 16:06

hugohaggmark reviewed Sep 24, 2020

View reviewed changes

torkelo approved these changes Sep 24, 2020

View reviewed changes

kaydelaney requested a review from a team September 24, 2020 14:18

kaydelaney force-pushed the datasource-services branch from 9c1cd0c to cdad148 Compare September 24, 2020 14:51

torkelo approved these changes Sep 25, 2020

View reviewed changes

hugohaggmark reviewed Sep 25, 2020

View reviewed changes

kaydelaney force-pushed the datasource-services branch from 6a76fc7 to 06f9eb3 Compare September 25, 2020 14:51

kaydelaney requested a review from hugohaggmark September 28, 2020 07:37

hugohaggmark reviewed Sep 28, 2020

View reviewed changes

public/app/plugins/datasource/cloudwatch/query_parameter_ctrl.ts Show resolved Hide resolved

hugohaggmark reviewed Sep 28, 2020

View reviewed changes

hugohaggmark approved these changes Sep 28, 2020

View reviewed changes

kaydelaney added 4 commits September 28, 2020 13:03

Chore: Remove angular dependency from data sources

403e402

Removes default export for time and template srvs

686effc

Also uses @grafana/runtime versions of the interfaces where possible

Replace usage of internal templateSrv where possible

e845542

Use runtime templateSrv in a couple more places

f71135c

kaydelaney force-pushed the datasource-services branch from 06f9eb3 to f71135c Compare September 28, 2020 12:42

Merge branch 'master' into datasource-services

7e9de0c

kaydelaney merged commit 1854421 into master Oct 1, 2020

Frontend Platform Backlog automation moved this from In Review to Done Oct 1, 2020

kaydelaney deleted the datasource-services branch October 1, 2020 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore: Remove angular dependency from data sources #27735

Chore: Remove angular dependency from data sources #27735

kaydelaney commented Sep 23, 2020

hugohaggmark Sep 24, 2020 •

edited

hugohaggmark Sep 24, 2020

hugohaggmark left a comment •

edited

torkelo left a comment

kaydelaney commented Sep 24, 2020

torkelo commented Sep 24, 2020

torkelo commented Sep 24, 2020

torkelo left a comment

hugohaggmark commented Sep 25, 2020

dprokop commented Sep 25, 2020

hugohaggmark Sep 25, 2020 •

edited

hugohaggmark Sep 25, 2020

hugohaggmark commented Sep 25, 2020

hugohaggmark Sep 28, 2020

hugohaggmark Sep 28, 2020

hugohaggmark left a comment

Chore: Remove angular dependency from data sources #27735

Chore: Remove angular dependency from data sources #27735

Conversation

kaydelaney commented Sep 23, 2020

hugohaggmark Sep 24, 2020 • edited

Choose a reason for hiding this comment

hugohaggmark Sep 24, 2020

Choose a reason for hiding this comment

hugohaggmark left a comment • edited

Choose a reason for hiding this comment

torkelo left a comment

Choose a reason for hiding this comment

kaydelaney commented Sep 24, 2020

torkelo commented Sep 24, 2020

torkelo commented Sep 24, 2020

torkelo left a comment

Choose a reason for hiding this comment

hugohaggmark commented Sep 25, 2020

dprokop commented Sep 25, 2020

hugohaggmark Sep 25, 2020 • edited

Choose a reason for hiding this comment

hugohaggmark Sep 25, 2020

Choose a reason for hiding this comment

hugohaggmark commented Sep 25, 2020

hugohaggmark Sep 28, 2020

Choose a reason for hiding this comment

hugohaggmark Sep 28, 2020

Choose a reason for hiding this comment

hugohaggmark left a comment

Choose a reason for hiding this comment

hugohaggmark Sep 24, 2020 •

edited

hugohaggmark left a comment •

edited

hugohaggmark Sep 25, 2020 •

edited