-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
First steps of RPC implementation for table page #3704
Conversation
f17ed79
to
07ef1c7
Compare
292db28
to
1814459
Compare
8043a0c
to
d3ad0cc
Compare
This was causing type errors in front end tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seancolsen I've looked through the code and it looks good. I have not QA tested the changes thoroughly, as you've mentioned that there are bugs in the description.
The api/ui/v0/connections/<id>/types/
endpoint is not working anymore (it works if id
=1, but not for other values) and the frontend isn't using the newly implemented types rpc endpoint yet. This is causing the data type of columns to be unknown. This is expected, I'm mentioning it here assuming it might help you prioritize items.
|
||
// TODO_BETA | ||
// | ||
// We need to find another way to get the valid target types for a column | ||
// since the RPC API no longer returns this. | ||
|
||
// column.valid_target_types ?? [], | ||
[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue created to keep track of this?
// TODO_BETA Do we need shareConsumer here? Previously we had been | ||
// passing: | ||
// | ||
// ``` | ||
// ...this.shareConsumer?.getQueryParams() | ||
// ``` | ||
this.promise = api.constraints.list(this.apiContext).run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will depend on the changes have to make for publicly shared links. The consumer here provides a shared-link-uuid
query param which used to get validated for each request made to any publicly shared table (& it's related APIs).
This PR is the first of many PRs that will move the table page from REST API calls to RPC API calls.
It handles:
columns.list
constraints.list
records.list
It doesn't handle:
tables.list_joinable
records.list
Already I can spot some issues, such as:
There are probably loads of other issues and bugs as well. But I'd like to do more thorough QA and polish later. For now I want to get this in and continue with the broad strokes so that we can better distribute and prioritize polish tasks later.
Checklist
Update index.md
).develop
branch of the repositoryDeveloper Certificate of Origin
Developer Certificate of Origin