-
Notifications
You must be signed in to change notification settings - Fork 173
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
chore: Update table path param to be a query param #1798
Conversation
✅ Deploy Preview for electric-next ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Looks good!
@KyleAMathews I've fixed up all the elixir, integration, and TS tests and any type issues in the examples - I haven't run all the examples to make sure they work though.
I've also updated the OpenAPI spec and some docs to reflect the API change and added "breaking" changesets.
I've noticed that we're using "root table" as a term in some places in the docs with respect to the API, and although the term makes sense for the shape definition I'm worried about it becoming confusing. I've made it so that 400 errors return with "table": ["table not found"]
rather than "root_table"
cause I feel that 400s should return errors matching the problematic parameters, but just noting this occasional discrepancy.
Great thanks! I'll give this a through go over later & double-check docs, examples, etc. and then merge it in |
Yeah |
Ok, this is ready to go — all examples are tested & gave another look over docs. Per #1796 (review) we'll just either need to decide on the order of merging. |
Not sure where those typescript errors are from — not reproducing locally. |
So everywhere we have table in the URL we need to switch to table as a param? Can you shout when this is merged so we can update parallel PRs like deployment docs and Elixir client. |
The docs are already updated as part of this PR — we can just release it right away when we merge. |
@KyleAMathews no need to rebase this as @thruflo has combined all these changes to #1900 |
…params (#1900) Included renames: - `electric-chunk-last-offset` -> `electric-offset` - `electric-next-cursor` -> `electric-cursor` - `electric-shape-id` -> `electric-handle` - `?shape_id` -> `?handle` Added query parameter: `?table` instead of `/:table` in the path Closes #1771, #1796, #1798 --------- Co-authored-by: Kyle Mathews <[email protected]> Co-authored-by: msfstef <[email protected]> Co-authored-by: Ilia Borovitinov <[email protected]>
Superseded by #1900 |
Partially addresses #1771