Refactoring React QueryBuilder to be uncontrolled #2303
Merged
+104
−85
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2303 +/- ##
=======================================
Coverage 55.74% 55.74%
=======================================
Files 118 118
Lines 8692 8692
Branches 1891 1891
=======================================
Hits 4845 4845
Misses 3468 3468
Partials 379 379
Continue to review full report at Codecov.
|
a1b4201
into
cube-js:master
17 of 18 checks passed
17 of 18 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Check List
Broken off from #2196
I ran into some issues while making other changes to the React QueryBuilder, and realized that it was because the QueryBuilder was neither fully controller nor uncontrolled, and was using the React anti-pattern of unconditionally copying props to state. Following the recommendations in that article and based on my experience as a React developer, I decided to make QueryBuilder uncontrolled. This means that QueryBuilder controls it's own state, and can't be modified from the outside save for in specific ways that are allowed through props.
This involved deprecating the
query
,setQuery
,vizState
, andsetVizState
props, and adding theinitialVizState
andonVizStateChanged
props. From my analysis, the most common use case for accessing thevizState
outside of the query editor is to save the editor state in persistent storage. These two new props allow for setting up a QueryBuilder component from a saved state and then listening for changes to save back. I'm using this in my app to save thevizState
to a database so I can persist the QueryBuilder state across reloads.I also added the
defaultQuery
anddefaultChartType
props, so that ifinitialVizState
is empty, it will pick up the default values for those.defaultChartType
was listed in the documentation before, but wasn't actually implemented until now.