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

Added cache prop to ResultSet #1552

Merged
merged 3 commits into from
Nov 7, 2016

Conversation

vera-liu
Copy link
Contributor

@vera-liu vera-liu commented Nov 7, 2016

Bug/Issue:

  • We don't want user to lose query results when they refresh the page
  • and VisualizeModal needs query results

Done:

  • Added cache prop to ResultSet
  • Results from query run by user are not flushed from global state, when user refreshes the page, results stay the same
  • Preview results of selected tables are stored in local component's state, it will go away when user refreshes.

needs-review @mistercrunch

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

As mentioned in the issue, the caching should target results.data instead of all of results

@@ -125,7 +127,8 @@ class ResultSet extends React.PureComponent {
}
render() {
const query = this.props.query;
const results = (this.props.query.cached) ? this.state.results : query.results;
const results =
(this.props.cache && this.props.query.cached) ? this.state.results : query.results;
Copy link
Member

Choose a reason for hiding this comment

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

nit this.props.query.cached -> query.cached

@vera-liu
Copy link
Contributor Author

vera-liu commented Nov 7, 2016

I think if we don't want user to lose query data after refreshing, we could just keep results in global state for queries run in editor, and store results locally only for data preview? @mistercrunch

@vera-liu
Copy link
Contributor Author

vera-liu commented Nov 7, 2016

Updated ResultSet to only cache results.data @mistercrunch

@mistercrunch
Copy link
Member

LGTM!

@vera-liu vera-liu merged commit 4014a48 into apache:master Nov 7, 2016
@vera-liu vera-liu deleted the vliu_no_cache_for_query_results branch November 14, 2016 21:32
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.13.0 labels Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants