React client TypeScript fixes, QueryBuilder improvements

[SteffeyDev]

Check List

• Tests has been run in packages where changes made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

Description of Changes Made

TypeScript type updates

I tried using cube.js react client in a typescript project, and found a number of issues with the provided typescript types. In many cases, the types were not specific enough to be used in a strict typescript project, but in others the types were simply incorrect. The included changes were necessary for me to use it in my project, where I use a I have created QueryBuilder wrapper similar to that in the playground, but strongly typed. I have also modified the types to support the QueryBuilder changes I made, as described below.

QueryBuilder Improvements

Convert to uncontrolled

I ran into some issues while making other improvements, 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 removing the query, setQuery, vizState, and setVizState props, and adding the initialVizState and onVizStateChanged props. From my analysis, the most common use case for accessing the vizState 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 the vizState to a database so I can persist the QueryBuilder state across reloads.

I also added the defaultQuery and defaultChartType props, so that if initialVizState 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.

Refactor orderMembers

Another issue I ran into was the interop between query.order and orderMembers. Whenever the query changed, orderMembers potentially needed to be updated, and every time orderMembers changed, the query might need to be updated. This relationship made reasoning about the various state changes difficult, and especially saving/restoring state and reloads.

I refactored the QueryBuilder so that orderMembers is calculated on demand in then render method based on the availableDimensions, availableMeasures, and query.order. Then, I changed updateOrder so that it just modified the query.order and sent it through the normal query update path. This makes query.order the single source of truth, and removes any difficulties with maintaining the relationship between query.order and orderMembers.

Add error render prop

I noticed that if dryRun failed, the error was logged to the console but not shown to the user. Instead of ignoring and letting the query call report the same error, I changed it so that dryRun errors are reported to the rendered function in the error prop. This is mostly useful when wrapWithQueryRenderer is false, as when it is true, the QueryRenderer will produce the same error and overwrite with it's own error prop (by design).

Add PropTypes

I added PropTypes so that non-typescript users can enjoy the benefit of prop type checking as well. This should be a lot safer and prevent unexpected props from being passed in.'

Summary

I know this is a lot, but I believe it makes the QueryBuilder a lot simpler to reason about, easier for fluent React developers like myself to pick it up and use it, and easier for future open source contributors to understand and add to the code.

I did not update the example React dashboard app and other docs yet with these changes, as I wanted to get approval and resolve all issues before I spent the time updating those. Once this is approved, I will go through and update the docs and examples to support these new props, and make sure the example react dashboard app still works.

As always, I'm hoping for good discussions and will readily make changes to address all concerns. I look forward to working with the cube.js development team on these changes, and if it goes well, would most likely invest more time into the project in the future.

[codecov]

Codecov Report

Merging #2196 (a8a6961) into master (d59bdfd) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2196      +/-   ##
==========================================
+ Coverage   55.73%   55.75%   +0.01%     
==========================================
  Files         118      118              
  Lines        8635     8641       +6     
  Branches     1888     1888              
==========================================
+ Hits         4813     4818       +5     
- Misses       3443     3444       +1     
  Partials      379      379              

Impacted Files	Coverage Δ
packages/cubejs-server-core/src/core/server.ts	50.46% <0.00%> (-0.95%)	⬇️
...es/cubejs-schema-compiler/src/adapter/BaseQuery.js	51.41% <0.00%> (+0.44%)	⬆️

---

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 d59bdfd...a8a6961. Read the comment docs.

[SteffeyDev]

Not sure why that integration test failed, but obviously nothing to do with this PR

[SteffeyDev]

I'll resolve conflicts once the PR is reviewed

[vasilev-alex]

Hi @SteffeyDev! Thanks for a great PR!

As I can see it introduces breaking changes which we cannot accept. However, we're ok with deprecating things but keeping them backward compatible for now.

Also, maybe you could consider splitting the PR? It'll make it easier to review and accept the changes.

As for PropTypes not sure if need it as we'll end up maintaining both index.d.ts and prop-types definitions. Moreover, all modern IDEs will take the TS definitions we have into account even for non-TS projects.

What do you think?

[SteffeyDev]

Concerning PropTypes, I think it's just a safer option, but definitely not needed. If you don't include them, there's always a risk that someone using an older editor might pass in invalid options, which causes internal issues and then that user gets frustrated because it looks like it is the component's fault, not theirs. Up to you as a maintainer.

[SteffeyDev]

After giving it some thought, I think I've found a way to make it backwards compatible with minimal consequences. I could get to work on that. While I'm doing that, I might also go ahead and split this into a few PRs. It will take a lot of time, but I understand why.

Here would be the 5 resulting PRs:

• Typescript type fixes
• Convert QueryBuilder to uncontrolled
• Refactor orderMembers
• Add queryError handling from dryRun
• Add PropTypes to QueryBuilder

Before I spend all the time on these, are any of these PRs that you would reject or have no interest in? I'll only spend the time on PRs that will be accepted (potentially after discussion and modifications).

[vasilev-alex]

@SteffeyDev, we're definitely willing to accept all of the PRs. But again, the main concern is that all changes to the API must be backward compatible.

Thanks for your time!

[SteffeyDev]

@vasilev-alex I created #2261, #2262, and #2263. Do you want me to create a PR for adding PropTypes, or do you not want to have the burden of maintaining PropTypes? Makes little difference to me.

Once those are merged in, I'll work on the bigger PR, which is converting to uncontrolled. That needs to build on top of these three.

[vasilev-alex]

@SteffeyDev nice! Let's go without PropTypes for now.