Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat: split superset-ui/query from superset-ui/chart #178

Merged
merged 6 commits into from
Aug 13, 2019

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Jun 18, 2019

💔 Breaking Changes

A. Some functions and types are removed from @superset-ui/chart and moved to new package @superset-ui/query

  • buildQueryContext
  • buildQueryObject
  • types related to QueryObject

There is no change to the functionalities.

Fix #176

B. ChartFormData is renamed to QueryFormData

This is to better reflect what the object actually is. Historically, formData in Superset stores all information necessary to reproduce a chart, which include

  1. database query information
  2. visual appearance

We have always considered formData to be a single type that contains both information in the context of Superset app (incubator-superset). However, after the development of embeddable charts, we see the following use cases:

  1. Query data from Superset without using its chart. => only need the query part of formData.
  2. Display Superset chart without query (User supplies data in their own way.) => only need the visual appearance part of formData.

It seems more logical to separate the two parts of formData into its own type.

  1. QueryFormData, which is what @superset-ui/query handles. buildQueryObject converts QueryFormData to QueryObject.
  2. ChartFormData, which varies chart by chart and can be defined in the @superset-ui/plugins packages.
type SupersetFormData = QueryFormData & ChartFormData

Currently, ChartFormData is referenced in plugin-chart-table which is still under development, and dataportal

Migration guide

  • Change broken reference from @superset-ui/chart to @superset-ui/query
  • For apps that has @superset-ui/chart as dependency, also add @superset-ui/query
  • Rename ChartFormData to QueryFormData.

@kristw kristw requested a review from a team as a code owner June 18, 2019 02:44
@codecov
Copy link

codecov bot commented Jun 18, 2019

Codecov Report

Merging #178 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #178   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         102      102           
  Lines        1221     1221           
  Branches      293      293           
=======================================
  Hits         1220     1220           
  Partials        1        1
Impacted Files Coverage Δ
...ckages/superset-ui-chart/src/models/ChartPlugin.ts 100% <ø> (ø) ⬆️
packages/superset-ui-query/src/convertMetric.ts 100% <ø> (ø)
packages/superset-ui-query/src/convertFilter.ts 100% <ø> (ø)
...src/registries/ChartBuildQueryRegistrySingleton.ts 100% <ø> (ø) ⬆️
packages/superset-ui-query/src/processMetrics.ts 100% <ø> (ø)
packages/superset-ui-query/src/processExtras.ts 100% <ø> (ø)
packages/superset-ui-query/src/types/Filter.ts 100% <ø> (ø)
...kages/superset-ui-query/src/types/QueryFormData.ts 100% <ø> (ø)
...rset-ui-chart/src/components/ChartDataProvider.tsx 100% <ø> (ø) ⬆️
packages/superset-ui-query/src/processFilters.ts 100% <ø> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8d7042...8b528de. Read the comment docs.

@kristw kristw changed the title Kristw query package feat: split superset-ui/query from superset-ui/chart Jun 18, 2019
@netlify
Copy link

netlify bot commented Jun 18, 2019

Deploy preview for superset-ui ready!

Built with commit 0b8613e

https://deploy-preview-178--superset-ui.netlify.com

@netlify
Copy link

netlify bot commented Jun 18, 2019

Deploy preview for superset-ui ready!

Built with commit 8b528de

https://deploy-preview-178--superset-ui.netlify.com

@kristw kristw added #code-quality reviewable Ready for review labels Jun 18, 2019
Copy link
Contributor

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

one question

import getChartBuildQueryRegistry from '../registries/ChartBuildQueryRegistrySingleton';
import getChartMetadataRegistry from '../registries/ChartMetadataRegistrySingleton';
import { AnnotationLayerMetadata } from '../types/Annotation';
Copy link
Contributor

Choose a reason for hiding this comment

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

Judging from the name, AnnotationLayerMetadata seems more related to charts than queries. why is it being pulled out here?

@kristw
Copy link
Contributor Author

kristw commented Jul 25, 2019

@etr2460 I have put the AnnotationLayerMetadata back

Copy link
Contributor

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

lgtm then!

@kristw kristw force-pushed the kristw--query-package branch from 6ecff97 to 8b528de Compare August 13, 2019 18:29
@kristw kristw changed the title feat: split superset-ui/query from superset-ui/chart breaking: split superset-ui/query from superset-ui/chart Aug 13, 2019
@kristw kristw changed the title breaking: split superset-ui/query from superset-ui/chart BREAKING: split superset-ui/query from superset-ui/chart Aug 13, 2019
@kristw kristw changed the title BREAKING: split superset-ui/query from superset-ui/chart feat: split superset-ui/query from superset-ui/chart Aug 13, 2019
@kristw kristw merged commit 630d3e5 into master Aug 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the kristw--query-package branch August 13, 2019 19:50
kristw added a commit that referenced this pull request Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split @superset-ui/query from @superset-ui/chart
2 participants